bpo-2506: Add mechanism to disable optimizations #22027
Conversation
2e9b121 to
1699ba0
Compare
|
Here we go again....:) |
d32ec2a to
45bbaef
Compare
There was a problem hiding this comment.
My brain always fail to understand a double negation: noopt=False. What about:
use_optimization=Truewith_optimization=Trueenable_optimization=Truedisable_optimization=False(still double negation)optimization=True... the annoying part with this one is that compile() already hasoptimizeparameter and there is alreadysys.flags.optimize, whereas cache_from_source() already hasoptimization:-(
To me, with_optimization=True makes sense (optimize), as well as with_optimization=False (don't optimize).
with fits well with an action like "compile". But it doesn't fit well with the global sys.flags. Maybe use_optimization=True is better.
Add -X noopt command line option to disable compiler optimizations. distutils ignores noopt: always enable compiler optimizations. * Add sys.flags.noopt flag. * Add 'noopt' keyword-only parameter to builtin compile(), importlib.util.cache_from_source() and py_compile.compile(). * importlib: SourceLoader gets an additional keyword-only '_noopt' parameter. It is used by py_compile.compile(). * importlib uses ``.noopt.pyc`` suffix for .pyc files if sys.flags.noopt is true. * Add PyConfig.optimize parameter. * Update subprocess._optim_args_from_interpreter_flags(). * Add support.requires_compiler_optimizations() * Update unit tests which rely on compiler optimizations for noopt: add @support.requires_compiler_optimizations. * Rename c_optimize to c_optimization_level in compiler.c Co-Authored-By: Yury Selivanov <yury@magic.io> Co-Authored-By: Victor Stinner <vstinner@python.org>
I was considering doing this but the problem is that all options are too similar to Another option is to deprecate |
|
Idea, maybe to avoid this problem we can pass the disabling options as a compiler flag. This has the advantage that we don't need an extra parameter and feels a bit more justified IMHO. Unless anyone has a problem with this I will go ahead with this version. |
Slow deprecations and migrations never end and confuse everybody. It's too late to change the parameter name. I only see two options:
We can use a different API for public functions like compile() and the command line interface, and for the internal C implementation (use two parameters). Sadly, the optimization level is also passed as "optimization" in some public C API functions. |
I have opted to implement this as a new bit that can be passed to the flags parameter. The only downside is that is not possible to enable optimizations per-compile call if you deactivate them globally using |
|
@vstinner I would like to land this PR once and for all so it does not die in spacetime like the other two attempts. When you have some time, could you do a final review? We could address some minor details in other PRs but given the change, the important thing here is having the bulk of the work so reviewing other future PRs will be easier. The important things to consider:
|
|
@nedbat Please, could you build this patch and check that this works as expected on |
|
@pablogsal Thanks for pushing this forward. Is there a way to enable this with an environment variable? I'm not sure how best to use this during a test run. Can you find me on IRC? nedbat |
|
Also, coverage.py will need to take this flag into account where it already tries to understand that optimizations might happen. If I hard-code "noopt" in the interpreter, the coverage.py test suite gets these failures: |
Sorry, I am not sure I understand what I am reading. Are these failures due to some code in |
|
Those tests are checks that coverage.py is correctly understanding the optimizations that happen. With "noopt", the tests fail, because the optimizations aren't happening. Coverage.py would need to take "noopt" into account when reporting on what lines could have run. |
|
Digging into this, I'm beginning to think the unintended consequences are not worth the change. Definitely this will need an environment variable to control it if it makes it to master. |
We could add the environment variable, but could you elaborate both points? What unintended consequences? Why you need an environment variable? |
Unintended ConsequencesCoverage.py works in three main phases:
Keep in mind, "lines" here really means, lines or transitions between lines in the case of branch coverage. When determining what lines of code could possibly be run, coverage.py takes into account the version of Python, and the optimizations done by that version. For example (taken from a test in the coverage.py test suite): In Python up until 3.8, there is a line event traced on line 2, setting up the loop. In Python 3.8, this was optimized away, and line 1 is followed by line 3. Coverage.py understands that this will happen. If it did not, it would treat 3.7 and 3.8 the same, and on 3.8 would report that line 2 didn't run when it should have. With Another example (also from test_arcs.py): Normally the "if 0" is optimized away, and lines 4, 5, and 6 are completely missing from the compiled code. With noopt, they are all present. I haven't finished working on this case, so I'm not sure how much adaptation it will need, but something will need to be done. Environment Variable supportWe need an environment variable to enable "noopt" because Python code, and in particular, test suites, get run too many different ways to assume that it will literally be a "python" command, and that an option can be provided to it. The command running the test suite might be "pytest", or "coverage run". The pytest runner can use xdist to spawn multiple processes to run tests in parallel. xdist will copy the environment variable from the parent, but doesn't know about "noopt". With an environment variable, I can run my pytest-xdist test runner entirely without optimization. Without an environment variable, I cannot. Providing an environment variable offers the greatest flexibility when running test suites. |
|
pablogsal#4 adds the environment variable. |
PYTHONNOOPT=1 will set the noopt flag
This adds some maintenance burden for us but it seems that it may add even more to |
|
Btw, thanks for the thorough explanation about the unintended consequences and why the env var is needed |
|
I think PEP 626 makes this unnecessary. |
I don't think so. I have to note that I the more I iterate over this the more I realize that this is quite a complex task with no much gain, honestly. Is not even possible to have an API that I am happy about. |
|
Mark, I appreciate your attention to this, but please, let's figure out where we end up with PEP 626 before deciding. Honestly, I've been wondering whether going back to this pull request might not be the way to go. |
https://bugs.python.org/issue2506