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

BLD: automatically detect if in freethread build of Python #158

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

tacaswell
Copy link
Contributor

setup.py Outdated
# build with Py_LIMITED_API unless explicitly disabled
USE_PY_LIMITED_API = os.getenv("PYERFA_LIMITED_API", "1") != "0"
USE_PY_LIMITED_API = (
(not sysconfig.get_config_var("Py_GIL_DISABLED")) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The link you give makes sense, but I'm still confused - this piece is about building, not running, and might we not want to build without the limited API on a regular, GIL-enabled python? cc @neutrinoceros, since he just put this in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know of a reason to build with the GIL and without Limited API, but the reason I specifically didn't implement this automatic switch is that it's only expected to be relevant for Python 3.13t, as 3.14t is expected to support the Limited API. That said, I made this choice before the guide had a recommendation there (or maybe before I learned about it), and maybe it's easier to follow a convention than having our own solution in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intent with this change was to preserve the ability to build with GIL and with not-limited.

As a random person trying to build packages (https://github.com/tacaswell/build_the_world), not having to set an pyerfa specific ENV is very convenient!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that makes sense - thanks, @neutrinoceros and @tacaswell!

@neutrinoceros - do we still need your special variable?

Copy link
Contributor

@neutrinoceros neutrinoceros Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My env var not used anywhere yet, and @tacaswell´s argument makes a lot of sense, so I say let's roll with it. Thanks !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neutrinoceros - So, remove your environment variable then in favor of this? (Right now, setting the environment variable is still kept as an option.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tacaswell - are you able to make the small change requested? (Remove support for @neutrinoceros's environment variable.) If not, we can do it.

@tacaswell
Copy link
Contributor Author

I made the code changes, but do not have time today to track down if there are doc changes that need to be made as well.

I have no problem with people pushing to this branch / closing this and re-creating a new PR. My to-do list is always longer than I have time to actually do so do not wait for me!

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is enough - the environment variable was not used anywhere in the repository yet.

@mhvk mhvk merged commit 16d8247 into liberfa:main Sep 16, 2024
24 checks passed
@tacaswell tacaswell deleted the bld/auto_nogil branch September 18, 2024 17:24
@neutrinoceros
Copy link
Contributor

@mhvk downstream is starting to hit build issues with cp313: https://github.com/cds-astro/mocpy/actions/runs/11345966890/job/31553997210
(in this case it seems devs didn't intend to use free-threaded Python 3.13 but they hit it anyway)
It might be helpful to cut a patch release with this fix.

@mhvk
Copy link
Contributor

mhvk commented Nov 11, 2024

@neutrinoceros - I had been waiting for a new SOFA release, but if there are problems that can be solved now, obviously we should go ahead, so I made a point release now. Thanks for the heads-up!

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

Successfully merging this pull request may close these issues.

3 participants