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

gh-108223: Refer to PEP 703 as Free Threading #112780

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Dec 5, 2023

Rename support.Py_GIL_DISABLED to support.FREE_THREADING.


📚 Documentation preview 📚: https://cpython-previews--112780.org.readthedocs.build/

@vstinner
Copy link
Member Author

vstinner commented Dec 5, 2023

cc @colesbury @corona10

Should the Py_GIL_DISABLED macro be renamed to Py_FREE_THREADING?

See the steering council decision: python/steering-council#221 (comment)

@brettcannon brettcannon removed their request for review December 5, 2023 23:23
@corona10
Copy link
Member

corona10 commented Dec 5, 2023

cc @hugovk

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

We already changed the term from Py_NOGIL to Py_GIL_DISABLED.
Do we have to touch this term all things one more time?

See PRs of #111863

@corona10
Copy link
Member

corona10 commented Dec 5, 2023

Ah okay, python/steering-council#221 (comment) was announced 2 hours ago.

@vstinner
Copy link
Member Author

vstinner commented Dec 5, 2023

Do we have to touch this term all things one more time?

Naming is hard :-) The update is that the Steering Council took a decision. But this PR is not about Py_GIL_DISABLED macro which can be renamed in a following PR if needed.

@itamaro
Copy link
Contributor

itamaro commented Dec 6, 2023

I'm confused. Didn't the SC decision explicitly mention that Py_GIL_DISABLED is ok?

@corona10
Copy link
Member

corona10 commented Dec 6, 2023

I have the same opinion as @itamaro
Except for updating build information from nogil to freethreading, the rest of the things do not need to be updated.

@hugovk
Copy link
Member

hugovk commented Dec 6, 2023

Should the Py_GIL_DISABLED macro be renamed to Py_FREE_THREADING?

See the steering council decision: python/steering-council#221 (comment)

No, we keep Py_GIL_DISABLED for the macro.

The SC said yesterday:

Within code, we can use technical implementation detail terms in names. Thus #214's Py_GIL_DISABLED #define and the --disable-gil configure flag making sense. Those are not typically exposed to users.

python/steering-council#221 (comment)

This PR doesn't touch the macro though. It changes a constant used by unit tests. This isn't exposed to users, so we could keep it as is, but are also free to change it.

However, because the constant is directly looking up the macro value from sysconfig, I'd think I'd prefer to keep them the same.

Doc/using/configure.rst Outdated Show resolved Hide resolved
@@ -291,7 +291,7 @@ def get_build_info():

# --disable-gil
if sysconfig.get_config_var('Py_GIL_DISABLED'):
build.append("nogil")
build.append("freethreading")
Copy link
Member

Choose a reason for hiding this comment

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

?

Suggested change
build.append("freethreading")
build.append("gil_disabled")

@vstinner
Copy link
Member Author

vstinner commented Dec 6, 2023

I updated the PR. Please review it again.

No, we keep Py_GIL_DISABLED for the macro.

Oh ok. I read the announcement too quickly, I missed the part about Py_GIL_DISABLED.

However, because the constant is directly looking up the macro value from sysconfig, I'd think I'd prefer to keep them the same.

Ok, I updated my PR for that.

@hugovk proposed build.append("gil_disabled"). From what I understood, anything displayed to the user should not associate free threading with negative term such as "disabled". I prefer to reuse the "official term": "free threading". I added an underscore for readability: build.append("free_threading").

@vstinner vstinner merged commit f885263 into python:main Dec 6, 2023
36 checks passed
@vstinner vstinner deleted the nonogil branch December 6, 2023 11:55
@vstinner
Copy link
Member Author

vstinner commented Dec 6, 2023

Merged, thanks for reviews.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants