Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST: Option to disable using C-extension for debugging #268

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pllim
Copy link
Contributor

@pllim pllim commented May 7, 2024

Maybe sometimes we want to see if tests behave the same with or without C-extensions.

Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.35%. Comparing base (49f8c89) to head (8424bd7).

Files Patch % Lines
spherical_geometry/__init__.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
+ Coverage   80.33%   80.35%   +0.01%     
==========================================
  Files           5        5              
  Lines        1017     1023       +6     
==========================================
+ Hits          817      822       +5     
- Misses        200      201       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tox.ini Outdated Show resolved Hide resolved
@pllim

This comment was marked as resolved.

@pllim
Copy link
Contributor Author

pllim commented May 7, 2024

OK now the jobs ignoring C-ext sees the errors #265 is trying to fix.

If you want this, please SQUASH AND MERGE.

@pllim pllim marked this pull request as ready for review May 7, 2024 19:30
@pllim pllim requested a review from a team as a code owner May 7, 2024 19:30
@pllim pllim requested a review from jhunkeler May 7, 2024 19:30
@mcara mcara added the maintenance Package maintenance label May 9, 2024
@mcara
Copy link
Member

mcara commented May 9, 2024

I think we need this. Thanks! However, I think I need to fix failing Python version of the C code first. So let's wait with merging a little.

pyproject.toml Outdated Show resolved Hide resolved
@pllim
Copy link
Contributor Author

pllim commented May 15, 2024

I rebased and cleaned things up. I think this is ready, but if #271 gets merged first, then I will have to rebase again.

@pllim
Copy link
Contributor Author

pllim commented May 15, 2024

For the record, another option I considered was introducing astropy.config for this package. It is more Pythonic, cleaner, and allow context manager, etc, but I decided that was way overkill for something that is only used for debugging.

Also see: https://docs.astropy.org/en/latest/config/index.html

@mcara
Copy link
Member

mcara commented May 15, 2024

Wouldn't it be easier to just not build math_util in setup.py?

@pllim
Copy link
Contributor Author

pllim commented May 15, 2024

just not build

How do you do that conditionally, on-demand?

@pllim pllim force-pushed the disable-math-util branch from afb5485 to 8424bd7 Compare May 15, 2024 18:43
@jhunkeler
Copy link
Contributor

I think we need this.

I agree. The ability to test the python code and math_util functions without rebuilding the package will let us know immediately when one, the other, or both are failing. Declaring HAS_C_UFUNCS once in __init__.py is a nice because we don't have to track down try/except blocks if we want to change the import behavior in the future.

@mcara
Copy link
Member

mcara commented May 15, 2024

just not build

How do you do that conditionally, on-demand?

Modify Extension in:

spherical_geometry/setup.py

Lines 115 to 119 in 49f8c89

setup(
ext_modules=[
Extension('spherical_geometry.math_util', sources, **ext_info)
],
)

depending on the environment variable value

Requires re-building but isn't it what is done anyway for each CI test?

@pllim
Copy link
Contributor Author

pllim commented May 15, 2024

Re: #268 (comment)

I don't see how that is simpler than what I am proposing.

@pllim
Copy link
Contributor Author

pllim commented May 15, 2024

Also, what I am proposing here allows you to test with and without C-extension locally without rebuilding, as @jhunkeler said above.

@mcara
Copy link
Member

mcara commented May 15, 2024

Re: #268 (comment)

I don't see how that is simpler than what I am proposing.

Because it is a single change in a single module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Package maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants