gh-109595: Add -Xcpu_count= cmdline for container users by corona10 · Pull Request #109667 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation90 Commits71 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Member
•
edited by github-actionsbot
Loading
Please read the issue of why this flag is needed and see also actual use-case from #109595 (comment)
Even if the container system is important these days, there is no way to limit CPU resources for the container environment from Python program side.
JDK provides -XX:ActiveProcessorCount=<n> from JDK 21 instead of supporting cgroup, this patch will provide identical feature to Python user and super useful :)
Co-authored-by: Victor Stinner vstinner@python.org
Co-authored-by: Victor Stinner vstinner@python.org
| @@ -715,6 +715,7 @@ static int test_init_from_config(void) |
|---|
| putenv("PYTHONINTMAXSTRDIGITS=6666"); |
| config.int_max_str_digits = 31337; |
| config.cpu_count = -1; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please set a more interesting value like 1234?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please document the change in Doc/whatsnew/3.13.rst?
Comment on lines 549 to 551
| * :samp:`-X cpu_count={n}` will override the number of CPU count from system. |
|---|
| If this option is provided, :func:`os.cpu_count` will return the overrided value. |
| And *n* must be greater equal then 1. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * :samp:`-X cpu_count={n}` will override the number of CPU count from system. |
|---|
| If this option is provided, :func:`os.cpu_count` will return the overrided value. |
| And *n* must be greater equal then 1. |
| * :samp:`-X cpu_count={n}` overrides the number of CPU count from system: |
| :func:`os.cpu_count` returns *n*. The value *n* must be greater than |
| or equal to 1. |
| static PyStatus |
|---|
| config_init_cpu_count(PyConfig *config) |
| { |
| int cpu_count = -1; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move the variable declaration inside the "if (sep)" block. Does it have to be initialized?
If you are afraid of undefined behavior, maybe config_wstr_to_int() should set *result to 0 on error.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I added PYTHONCPUCOUNT envvar, so the current structure will be needed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want, you can move the variable declaration inside each "if (env)" block and duplicate the variable, to better show its scope. But well, that's just a personal preference. Feel free to ignore my coding style remark ;-)
Co-authored-by: Victor Stinner vstinner@python.org
Co-authored-by: Victor Stinner vstinner@python.org
Lib/os.py Outdated Show resolved Hide resolved
Adds PYTHON_CPU_COUNT help text. Mentions multiprocessing.cpu_count. rewords a few statements.
@gpshead I will merge this PR by tomorrow. Please let me know if there needs more change. :)
cc @vstinner
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I see that you addressed the two @gpshead's comments. You can merge.
Follow-up: issue #110649 to add -X cpu_count=process.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request
Co-authored-by: Victor Stinner vstinner@python.org Co-authored-by: Gregory P. Smith [Google LLC] greg@krypto.org
Labels
bugs and security fixes
Documentation in the Doc dir
A feature request or enhancement