classification
Title: PYTHONOPTIMIZE ignored in 3.7.0 when using custom launcher
Type: behavior Stage: commit review
Components: Interpreter Core Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: vstinner Nosy List: brett.cannon, eric.snow, ncoghlan, ned.deily, ronaldoussoren, vstinner
Priority: release blocker Keywords: 3.7regression, patch

Created on 2018-07-27 12:07 by ronaldoussoren, last changed 2018-08-05 16:07 by ncoghlan.

Files
File name Uploaded Description Edit
t.c ronaldoussoren, 2018-07-27 12:07
foo.py ronaldoussoren, 2018-07-27 12:07
Pull Requests
URL Status Linked Edit
PR 8521 closed ronaldoussoren, 2018-07-28 12:36
PR 8659 merged vstinner, 2018-08-03 20:25
Messages (18)
msg322487 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2018-07-27 12:07
Attached is a very basic launcher for python scripts. This script works as expected in Python 3.6, but not in 3.7.0.

In particular, the launcher sets the environment variable PYTHONOPTIMIZE before calling Py_Initialize and that setting is ignored.

I haven't tried testing with the tip of the tree yet.
msg322519 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-28 03:25
This feels like a duplicate of https://bugs.python.org/issue31845 (see https://github.com/python/cpython/commit/d7ac06126db86f76ba92cbca4cb702852a321f78 )

Is this definitely with the 3.7.0 release, or is it potentially with one of the earlier alphas before that fix was applied?
msg322520 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-28 03:44
Ah, wait - I see a relevant difference: the test case for issue31845 uses Py_Main, *not* Py_Initialize (since it's a command line test, not an embedding test).

That means it's currently possible for the sequence of operations in https://github.com/python/cpython/blob/7cbde0e09daba4259565738e48f141851287fe29/Python/pylifecycle.c#L913 to be wrong (or missing steps) without causing the test suite to fail.
msg322521 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-28 03:56
Comparing to Python 3.6, we can see that the code to update the C global flags from the environment variables used to live directly in _PyInitializeEx_Private: https://github.com/python/cpython/blob/9d85856044a2c7d681ea38b5ff99af375b228a0f/Python/pylifecycle.c#L305

In Python 3.7 that section (https://github.com/python/cpython/blob/3.7/Python/pylifecycle.c#L913 ) currently calls _PyCoreConfig_Read instead, which *only* updates the config struct - it doesn't update the C level global variables.

By contrast, Py_Main eventually calls https://github.com/python/cpython/blob/7cbde0e09daba4259565738e48f141851287fe29/Modules/main.c#L1394 which handles writing the state from the config struct back to the global variables.
msg322522 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-28 04:00
Next steps:

- add a test case that runs the same sys.flags checks as in https://github.com/python/cpython/commit/d7ac06126db86f76ba92cbca4cb702852a321f78, but as a test_embed embedding test using Py_Initialize rather than Py_Main
- create a shared internal API that both Py_Main and Py_Initialize can use to write the environment variable settings back to the global variables
msg322523 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-28 04:04
Ned, I think we should consider this a release blocker for 3.7.1
msg322524 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-28 05:09
This seems closely related to the work Victor is already pursuing to resolve bpo-34170 for Python 3.8 (e.g. https://github.com/python/cpython/commit/56b29b6d6fa3eb32bb1533ee3f21b1e7135648a0 ).

The bpo-34170 changes are more invasive than we'd like for a maintenance release, but it would be preferable to keep at least any new Python 3.7 test cases somewhat aligned with their Python 3.8 counterparts.
msg322529 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2018-07-28 09:13
I'm at the EuroPython sprints and currently working on a testcase for this. Should be a nice way to get back into actively contributing to CPython :-)
msg322532 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2018-07-28 09:24
FYI: I've filed issue34255 while working on this, test_embed assumes that you're building in the source directory (which I usually don't do).
msg322533 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2018-07-28 09:52
Interesting... pylifecycle.c uses code in main.c, I hadn't expected that.

If I've read the code correctly Py_Initialize won't look at PYTHONOPTIMZE at all (and other environment variables that have a command-line equivalent). Those are read in main.c:cmdline_get_env_flags, which is only called (indirectly) from Py_Main and _Py_UnixMain. 

I'm note sure what's the correct solution for this.

1. Reading PYTHONOPTIMZE (and related variables) could be done 
   in _PyCoreConfig_Read, but that would then need to grow a flag to ensure
   that this won't replace values calculated earlier in the implementation 
   of Py_Main

2. Writing back the global variables is probably best done using a new
   function (lets _PyCoreConfig_SetGlobals), mirroring    
   pymain_set_global_config but using a _PyCoreConfig config structure
   instead of the command-line structure used by the latter function.
msg322550 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2018-07-28 12:39
I've created a pull request that seems to work properly, although I am not entirely sure that this is the right way to fix this.

As I mentioned in the pull request I created it against the 3.7 branch because of bpo-34170, but would happily rebase it for master.
msg322562 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-28 14:22
I believe Victor is at the EuroPython sprints as well, so if I'm right about that, I think the best option would be for the two of you to work out a resolution for 3.7.1 in person that at least keeps the test suites reasonably consistent, even if the implementations have diverged (the new tests already added for bpo-34170 were the main reason I stopped working on my initial patch for this).

And agreed on fixing the current dependency inversion between pylifecycle and the py_main code - the functions needed by both Py_Main and Py_Initialize should be migrated down the stack into pylifecycle as part of the bpo-34170 work.
msg323068 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-08-03 20:27
I first proposed to backport all code from master:
https://mail.python.org/pipermail/python-dev/2018-July/154882.html

But I'm not sure that it's a good idea, since I made *many* changes in the master branch since 3.7... I even added more files, and the _PyCoreConfig API is going to change anyway...

So I wrote a simpler PR 8659 which backports unit tests, adapt them for Python 3.7, and fix this issue: PYTHONOPTIMIZE and other environment variables and now read by Py_Initialize(), not only by Py_Main().
msg323070 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-08-03 20:35
I tested manually attached t.c with PR 8659: I get optimize=2 as expected. Full output:

sys.flags(debug=0, inspect=0, interactive=0, optimize=2, dont_write_bytecode=0, no_user_site=0, no_site=0, ignore_environment=0, verbose=0, bytes_warning=0, quiet=0, hash_randomization=1, isolated=0, dev_mode=False, utf8_mode=1)
msg323135 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2018-08-05 07:40
The PR works for me as well, and looks ok to me.
msg323144 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-08-05 10:32
New changeset 0c90d6f75931da4fec84d06c2efe9dd94bb96b77 by Victor Stinner in branch '3.7':
[3.7] bpo-34247: Fix Python 3.7 initialization (#8659)
https://github.com/python/cpython/commit/0c90d6f75931da4fec84d06c2efe9dd94bb96b77
msg323145 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-08-05 10:33
On my PR, Nick Coghlan wrote:

"We may want to tweak the porting note in What's New, as I believe this may read more environment variables at init time than the interpreter did previously, so embedding applications that want more complete control may now need to set Py_IgnoreEnvironmentFlag when they could previously get by without it."

https://github.com/python/cpython/pull/8659#pullrequestreview-143397378

I'm not sure about this, since it's really a bug of Python 3.7.0 and it's now fixed.

I let you decide if the issue should be closed or not, I leave it open.
msg323156 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-08-05 16:07
My comment wasn't about the 3.7.0 -> 3.7.1 fix, but rather about the fact that I suspect that 3.7.1 is now going to respect some environment variables in this case that 3.6.x ignored. I don't know for sure though, since we didn't have a test case for this until you wrote one.

So I was thinking we could include a porting note along the lines of:

===============
- starting in 3.7.1, `Py_Initialize` now consistently reads and respects all of the same environment settings as `Py_Main` (in earlier Python versions, it respected an ill-defined subset of those environment variables, while in Python 3.7.0 it didn't read any of them due to :issue:`34247`). If this behaviour is unwanted, set `Py_IgnoreEnvironmentFlag = 1` before calling `Py_Initialize`.
===============

That note then serves two purposes:

- it lets embedders know that `Py_IgnoreEnvironmentFlag = 0` just plain doesn't work properly in 3.7.0 (without their needing to find this bug for themselves)
- it lets embedders know that they may want to set `Py_IgnoreEnvironmentFlag = 1` if their embedded interpreter is now being affected by environmental settings that it ignored in previous versions
History
Date User Action Args
2018-08-05 16:07:17ncoghlansetmessages: + msg323156
stage: patch review -> commit review
2018-08-05 11:45:42serhiy.storchakasettitle: PYTHONOPTIMZE ignored in 3.7.0 when using custom launcher -> PYTHONOPTIMIZE ignored in 3.7.0 when using custom launcher
2018-08-05 10:33:50vstinnersetmessages: + msg323145
2018-08-05 10:32:04vstinnersetmessages: + msg323144
2018-08-05 07:40:09ronaldoussorensetmessages: + msg323135
2018-08-03 20:35:30vstinnersetmessages: + msg323070
2018-08-03 20:27:52vstinnersetmessages: + msg323068
2018-08-03 20:25:08vstinnersetpull_requests: + pull_request8153
2018-07-28 14:22:57ncoghlansetmessages: + msg322562
2018-07-28 12:39:34ronaldoussorensetmessages: + msg322550
2018-07-28 12:36:12ronaldoussorensetkeywords: + patch
stage: patch review
pull_requests: + pull_request8037
2018-07-28 09:52:51ronaldoussorensetmessages: + msg322533
2018-07-28 09:24:48ronaldoussorensetmessages: + msg322532
2018-07-28 09:13:43ronaldoussorensetmessages: + msg322529
2018-07-28 05:09:18ncoghlansetassignee: vstinner
messages: + msg322524
2018-07-28 04:04:44ncoghlansetpriority: normal -> release blocker
nosy: + ned.deily
messages: + msg322523

2018-07-28 04:00:08ncoghlansetmessages: + msg322522
2018-07-28 03:56:39ncoghlansetmessages: + msg322521
2018-07-28 03:44:06ncoghlansetmessages: + msg322520
2018-07-28 03:25:39ncoghlansetnosy: + vstinner
messages: + msg322519
2018-07-27 17:30:45serhiy.storchakasetnosy: + brett.cannon, ncoghlan, eric.snow
2018-07-27 12:07:53ronaldoussorensetfiles: + foo.py
2018-07-27 12:07:40ronaldoussorensetfiles: + t.c
2018-07-27 12:07:30ronaldoussorencreate