-
Notifications
You must be signed in to change notification settings - Fork 212
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
fix: detect gil #2762
fix: detect gil #2762
Conversation
@@ -159,10 +160,20 @@ fn get_implementation_name(python_record: &PackageRecord) -> miette::Result<&'st | |||
} | |||
} | |||
|
|||
/// Return whether the specified record has gil disabled (by being a free-threaded python interpreter) | |||
/// by looking into the build string of the record. | |||
fn gil_disabled(python_record: &PackageRecord) -> miette::Result<bool> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we can depend on that t
I've asked in the conda-forge zulip: https://conda-forge.zulipchat.com/#narrow/channel/457337-general/topic/Python.20freethreading.20from.20record/near/490528853
Also, I think you can skip the result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Let's wait for the resolution from their side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on discussion with @wolfv - it seems right way to do things, we just need to additional check if it's from conda-forge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didnt follow the discussion with wolf but isnt isurufs answer on zulip the rught approach for the tags?
Then
python_abi
is the correct way. You don't have to check that it ends witht
. Just use the entirety ofcp313t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didnt follow the discussion with wolf but isnt isurufs answer on zulip the rught approach for the tags?
Then
python_abi
is the correct way. You don't have to check that it ends witht
. Just use the entirety ofcp313t
yes! isurufs answer is the right one - we just discussed it before any answers on zulip
// In order to detect if the python interpreter is free-threaded, we look at the depends | ||
// field of the record. If the record has a dependency on `python_abi`, then | ||
// look at the build string to detect cpXXXt (free-threaded python interpreter). | ||
let regex = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wont it be enough to just check if it starts with cp
and ends with t
?
If not, please use a OnceLock
to lazily initialize the regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be part of something like this: *_cp313t
so I belive that the regex is more approriate solution here
Overview
Fixes #2732 by detecting if it's a free-threaded python as using it to configure the Tags.
p.s. you still need to use
as pointed by @ruben-arts