Skip to content

support.args_from_interpreter_flags() adds -X uops #110124

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

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

No description provided.

@vstinner
Copy link
Member Author

@gvanrossum: -X uops is not documented, is it on purpose?

With this change, the following tests will inherit -X uops as expected:

$ git grep -l args_from_interpreter_flags
Doc/library/test.rst
Lib/multiprocessing/forkserver.py
Lib/multiprocessing/resource_tracker.py
Lib/multiprocessing/spawn.py
Lib/multiprocessing/util.py
Lib/subprocess.py
Lib/test/bisect_cmd.py
Lib/test/libregrtest/worker.py
Lib/test/support/__init__.py
Lib/test/test_cmd_line_script.py
Lib/test/test_compileall.py
Lib/test/test_gdb/util.py
Lib/test/test_inspect.py
Lib/test/test_support.py
Tools/scripts/run_tests.py

Currently, it's only done manually in test_regrtest.py.

@gvanrossum
Copy link
Member

I'm torn. I didn't know there was infrastructure for passing -X flags that could be easily extended. Then again I've not needed to run those tests with -Xuops. And when I want to run absolutely everything with -Xuops I use a one-line hack that turns it on by default.

It is not our intention that end users or even contributors (including most core devs) would ever need to pass -Xuops -- this is a temporary hack until we can enable the Tier 2 interpreter for everyone always. If anything, at that point we might have to provide a way to disable Tier 2 for some rare use cases (like we offered a way to revert to the old parser when we introduced the PEG parser, for one release). But what form that would take I don't know yet.

I have no idea how the code you're changing here works -- if you need it yourself I am fine with you merging this, but I don't feel comfortable approving the PR.

@vstinner
Copy link
Member Author

I have no idea how the code you're changing here works -- if you need it yourself I am fine with you merging this, but I don't feel comfortable approving the PR.

Usually, regrtest tries to pass Python command line options of the first command (typed by the user) to worker processes. It's just convenient:

./python -X uops -m test test_sys -j1 runs ./python -X uops -m test.libregrtest.worker test_sys command with this change.

Without this change, -X uops is ignored and ./python -m test.libregrtest.worker test_sys is run instead.

I don't need this options. I'm not sure that I understand its purpose (since it's not documented ;-)). It's just that I was surprised to see a special code path for it in test_regtest, and only in test_regrtest.

If the option should not be passed to test worker processes, should the test_regrtest change be reverted? I'm talking about these lines:

    def run_python(self, args, **kw):
        extraargs = []
        if 'uops' in sys._xoptions:
            # Pass -X uops along
            extraargs.extend(['-X', 'uops'])
        args = [sys.executable, *extraargs, '-X', 'faulthandler', '-I', *args]

For this code, I chose to not use support.args_from_interpreter_flags() nor support.optim_args_from_interpreter_flags(). I prefer to control how tests are run. By the way, regrtest itself now adds options to Python when --fast-ci or --slow-ci option is used.

@gvanrossum
Copy link
Member

You can look at the git history to see why I added that. I’m sure I needed it for something.

@gvanrossum
Copy link
Member

In Brno I can explain the option.

@vstinner
Copy link
Member Author

In short, I understand that this change is not desired, the current behavior is the expected behavior. Ok, I just close my issue. Thanks for the explanation.

@vstinner vstinner closed this Sep 30, 2023
@vstinner vstinner deleted the args_uops branch September 30, 2023 17:31
@gvanrossum
Copy link
Member

Thanks. Cross-reference: #108778

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.

2 participants