classification
Title: Benchmark runner tries to execute external Python command and fails on error reporting
Type: behavior Stage: needs patch
Components: Benchmarks Versions:
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, lemburg, python-dev, r.david.murray, scoder, vstinner
Priority: normal Keywords: patch

Created on 2013-09-27 16:00 by scoder, last changed 2016-09-03 05:35 by scoder. This issue is now closed.

Files
File name Uploaded Description Edit
add_pyversions_option_and_refactor_runtime_config.patch scoder, 2013-10-12 15:52
Messages (19)
msg198487 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2013-09-27 16:00
In changeset 88b6ef9aa9e9, a new function ported_lib() was added that crashes on error reporting in Py3 because it tries to do this:

    raise RuntimeError("Benchmark died: " + err)

"err" is a bytes object that comes straight from the subprocess pipe.

The reason why I noticed this is because the new function assumes that it can run "python" with the "-c" option and pass an arbitrary Python command string. That may not be true if it is actually executing some wrapper script (as we do for Cython, for example). Currently, the only requirement for the external Python command is that it can run a Python script that it receives on the command line. We shouldn't unnecessarily extend that interface.

I'm not sure how to best fix this, but the only way I can currently see is to run an external Python script that prints the requested information.
msg198534 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-09-28 17:16
New changeset 921177a65386 by Antoine Pitrou in branch 'default':
Issue #19108: fix str/bytes mismatch when raising exception under Python 3
http://hg.python.org/benchmarks/rev/921177a65386
msg198535 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-28 17:18
Ok, I've fixed the str/bytes mismatch. I don't have any strong feelings about the rest, other than whoever wants perf.py to work with wrapper scripts should probably propose a patch. I'll let Brett decide.
msg198536 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2013-09-28 17:20
Well, it worked before, so the current state is clearly a regression.
msg198755 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2013-10-01 08:39
Antoine, I really don't like this attitude of adding code and then saying "well, it's there, I won't change it" when others complain about breakage. Please undo your change that broke the ability of using (non-trivial) wrapper scripts for the Python runtimes. If you think that that change provides an important feature, then please implement that feature in a way that does not break existing usages of the benchmark runner.
msg198760 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-01 09:12
> Antoine, I really don't like this attitude of adding code and then
> saying "well, it's there, I won't change it" when others complain
> about breakage.

Well, I don't care whether you "like this attitude". Your own
attitude has irked several developers enough that they're more or
less ignoring you. That includes me. If you're not willing to
be respectful and constructive, the situation won't improve.

As for the present issue, the benchmark suite is meant to work
with an actual Python interpreter. Not a wrapper script that
can pretend to be one for a single invocation mode.

If you think otherwise, you can of course provide evidence, and
a patch to fix things.
(and hope for someone to apply the patch)
msg198761 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-01 09:13
Brett, I'm leaving you with this, if you're wanting to do anything about it :-)
msg198762 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2013-10-01 09:37
> I'm leaving you with this, if you're wanting to do anything about it

Sorry, but weren't you just asking *me* to be constructive?

I'm aware that getting this change right isn't trivial. But that doesn't mean we should happily break other people's code and then leave the cleanup to them. This isn't a green-field project, the code is actually being used by other projects than CPython. In fact, the whole purpose of this code is to be used by other projects as well. That means that there may be some additional restrictions on what we should expect from a benchmarked runtime. Previously, the requirements were "can run a Python script from the command line". Now, the additional requirements are "accepts the -c switch" and "can execute a line of Python code passed on the command line". That is substantially harder to handle in a script.
msg198764 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2013-10-01 10:40
Wouldn't it be possible to use the old version as fallback solution
in case the -c switch approach fails or have a command line option
to pass in the version in order to bypass all of this ?

Stefan: Why don't you propose a patch which implements this ?
msg198866 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2013-10-02 19:59
What about this: by default, we assume all runtimes to have the same major version as the Python runtime that executes the benchmark runner. If that's not the case, users must override it explicitly with a command line option, say, "--pyversions 2:3" for a reference 2.x and a comparison 3.x version of Python (i.e. in the order the programs appear on the command line).

Does that sound ok?

I'd like to avoid a trial-and-fallback approach as that would mean that any wrapper scripts would still have to support being called twice and handling the -c option in one way or another.
msg198867 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-10-02 20:06
If you want to propose a patch to add specifying the version on the command-line to avoid inferring the version I would be fine with that.
msg198894 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-10-03 13:39
I should mention any solution for the command-line should take a N.N value *only* and not just 2/3 for instances where tests do not work with the latest version of Python yet.
msg198898 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2013-10-03 14:17
I'm having trouble understanding your last comment. Are you saing that you want the exact value to be a two digits version and therefore use separate arguments for both Pythons (e.g. "--basever 2.7 --cmpver 3.3"), or that you want it to accept two digit versions instead of major-only regardless of the number of arguments (e.g. "--pyversion 2.7,3.3")?

And are you saying that you want to use the version also for, say, disabling a test because it doesn't run in Py3.4 yet, despite supporting Py3.3, for example?

A comma is what is used by "--args", BTW, that's why I'd reuse it for the versions argument, i.e. "--pyversion 2.7,3.3" and additionally "--pyversion 3.3" for the simple case.

I haven't come up with a patch yet because it requires a number of non-local changes throughout the file.
msg198899 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-10-03 15:16
I want to only accept major.minor version specifications; how that is done on the command-line I don't care and leave up to you.

And yes, the version may be used in the future to disable tests that e.g. don't work on Python 3.4 like Chameleon.
msg199579 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2013-10-12 15:52
Here's a patch that replaces the current simplistic Python executable command config with a dedicated PythonRuntime config class. That makes it easy to properly pass around the program specific configuration. Part of that is the Python executable path, the Python version, the specific command line arguments and the relative benchmark library path.

The patch also adds a "--pyversions" command line option to avoid calling the executable for figuring out the Python versions, as discussed.
msg207131 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2014-01-01 09:23
*bump* - patch pending review.
msg207138 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-01-01 18:54
The patch looks good to me, but since I'm not familiar with perf.py I'm not a good person to do a final review and commit it.

One trivial question: why do you check for tupleness in PythonRuntime's init?  Don't you control the input on both code paths to obtaining the version?
msg273929 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-30 16:26
The new https://github.com/python/performance benchmark suite has a very different design than the old https://hg.python.org/benchmarks benchmark suite:

* it now only runs benchmarks in a virtual environment
* dependencies are installed in the venv by pip
* there is a "run" command which now accepts a --python=PYTHON option to choose the Python binary (and so the tested Python version)

There is no more such thing as ported_lib().

Can I now close this issue as outdated, or do you consider that performance still has this issue?
msg274287 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2016-09-03 05:35
Let's close this as outdated. New bugs for the new project should be reported in github anyway.
History
Date User Action Args
2016-09-03 05:35:34scodersetstatus: open -> closed
resolution: out of date
messages: + msg274287
2016-08-30 16:26:14vstinnersetnosy: + vstinner
messages: + msg273929
2014-01-01 18:54:16r.david.murraysetnosy: + r.david.murray
messages: + msg207138
2014-01-01 09:23:56scodersetmessages: + msg207131
2013-10-12 15:52:54scodersetfiles: + add_pyversions_option_and_refactor_runtime_config.patch
keywords: + patch
messages: + msg199579
2013-10-03 15:16:44brett.cannonsetmessages: + msg198899
2013-10-03 14:17:16scodersetmessages: + msg198898
2013-10-03 13:39:24brett.cannonsetmessages: + msg198894
2013-10-02 20:06:11brett.cannonsetmessages: + msg198867
stage: needs patch
2013-10-02 19:59:25scodersetmessages: + msg198866
2013-10-01 10:40:38lemburgsetnosy: + lemburg
messages: + msg198764
2013-10-01 09:37:48scodersetmessages: + msg198762
2013-10-01 09:13:59pitrousetnosy: - pitrou
2013-10-01 09:13:54pitrousetnosy: brett.cannon, pitrou, scoder, python-dev
messages: + msg198761
2013-10-01 09:12:56pitrousetmessages: + msg198760
2013-10-01 08:39:30scodersetmessages: + msg198755
2013-09-28 17:20:50scodersetmessages: + msg198536
2013-09-28 17:18:21pitrousetmessages: + msg198535
2013-09-28 17:16:48python-devsetnosy: + python-dev
messages: + msg198534
2013-09-27 16:00:36scodercreate