Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Switch .py shebang to python3 #28963

Merged
merged 2 commits into from
Mar 14, 2022
Merged

Switch .py shebang to python3 #28963

merged 2 commits into from
Mar 14, 2022

Conversation

jtattermusch
Copy link
Contributor

Followup for python3 migration.

@jtattermusch jtattermusch requested a review from gnossen February 24, 2022 08:02
@jtattermusch jtattermusch added the release notes: no Indicates if PR should not be in release notes label Feb 24, 2022
@jtattermusch jtattermusch mentioned this pull request Feb 24, 2022
Merged
@jtattermusch
Copy link
Contributor Author

My guess is that the Python MacOS failure (https://source.cloud.google.com/results/invocations/0349f858-b297-4b22-b1f8-c23ef6cf2cf8/log) is the same as in #28125 (comment).

@gnossen is there any progress with debugging that problem?

@gnossen
Copy link
Contributor

gnossen commented Mar 8, 2022

Quick update. I was able to reproduce this locally and will continue debugging it today.

@jtattermusch jtattermusch mentioned this pull request Mar 9, 2022
Merged
@jtattermusch
Copy link
Contributor Author

Quick update. I was able to reproduce this locally and will continue debugging it today.

Thanks for the update. Please note that this is now blocking #28966

@gnossen
Copy link
Contributor

gnossen commented Mar 11, 2022

It turns out my local error was not a reproduction. And it looks like I'm unable to SSH to the Mac workers, so I've been forced to repeatedly commit, push, and wait. I don't have a complete fix yet, but I do know a lot more. It looks like this is related to an implementation detail of CPython on Mac OS. The __PYVENV_LAUNCHER__ environment variable points at 3.7 and every time Python spins up a subprocess, this seems to be the executable used, regardless of the Python version of the parent process.

I was able to get the build step to pass by unsetting this env var in the build step. But then all of the gRPC Python tests that spin up subprocesses started failing, saying that they couldn't find the grpc module. This likely means that those subprocesses were instantiated with 3.7 again, somehow. I'm not sure where the environment variable is getting reinjected. My most recent fix attempt was to set put the 3.8 installation (which seems to be a better behaved CPython) at the Kokoro entrypoint, but I'm still seeing issues with the tests that involve subprocesses.

@jtattermusch
Copy link
Contributor Author

@gnossen those are good findings! very helpful.
I did a bit more research and it seems that the issue you're describing above is https://bugs.python.org/issue22490 and based on
changelog it was fixed in python3.8.3 (it was also backported to python3.7)

Also, strangely enough, our "prepare prerequisites" script on macos installs python 3.7.0, 3.8.0, 3.9.0 and s.o. (thus not including patches for each of the versions, which seems to be a mistake).
e.g. python 3.8.0 (patch version 0) is installed here:

grpc/tools/internal_ci/helper_scripts/prepare_build_macos_rc

Line 136 in 4ffcef7

time curl -O https://www.python.org/ftp/python/3.8.0/python-3.8.0-macosx10.9.pkg

It seems that a straightforward fix might be to install the latest available patch version for all the pythons.

@jtattermusch
Copy link
Contributor Author

jtattermusch commented Mar 11, 2022 •
edited
Loading

Looks like the prepare_build_macos_rc change I added to this PR is fixing the problem with __PYVENV_LAUNCHER__: https://source.cloud.google.com/results/invocations/1a32ef1f-1963-4775-bdaa-17c9089348fa

FTR kokoro mac workers come with python3.8.2 pre-installed (which is the version just before the fix for https://bugs.python.org/issue22490 was introduced).

@jtattermusch
Copy link
Contributor Author

the fix from this PR should also unblock #28125

@jtattermusch
Copy link
Contributor Author

As the fix in this PR seems to be working correctly, I'm going to merge (I'm happy to address additional comments as a followup).

Known failures:

  • node interop test build is failing on also master.

@jtattermusch
Copy link
Contributor Author

CC @lidizheng

@jtattermusch jtattermusch merged commit a3be072 into grpc:master Mar 14, 2022
@jtattermusch jtattermusch mentioned this pull request Mar 14, 2022
Merged
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Mar 14, 2022
@jtattermusch jtattermusch mentioned this pull request Mar 18, 2022
Closed
lepistone added a commit to lepistone/grpc that referenced this pull request Nov 14, 2023
@lepistone
[benchmark] Use python3 for subprocesses, too
575d8df 
After grpc#28963, the top Python scripts run with the "python3" entry-point
found on the PATH.

It makes sense to use the same for subprocesses, for two reasons:

- There might be no "python" in the PATH, as in my Mac with brew
- Having a different Python for subprocesses, especially if it's a
  Python 2, is asking for trouble.
lepistone added a commit to lepistone/grpc that referenced this pull request Nov 14, 2023
@lepistone
[benchmark] Use python3 for subprocesses, too
93417a0 
After grpc#28963, the Python entry points run with the "python3" entry-point
found on the PATH.

It makes sense to use the same for subprocesses, for two reasons:

- There might be no "python" in the PATH, as in my Mac with brew
- Having a different Python for subprocesses, especially if it's a
  Python 2, is asking for trouble.
lepistone added a commit to lepistone/grpc that referenced this pull request Nov 14, 2023
@lepistone
[benchmark] Use python3 for subprocesses, too
2349ce6 
After grpc#28963, the Python entry points run with "python3" found on the
PATH.

It makes sense to use the same for subprocesses, for two reasons:

- There might be no "python" in the PATH, as in my Mac with brew
- Having a different Python for subprocesses, especially if it's a
  Python 2, is asking for trouble.
@lepistone lepistone mentioned this pull request Nov 14, 2023
Open
lepistone added a commit to lepistone/grpc that referenced this pull request Nov 23, 2023
@lepistone
[benchmark] Use python3 for subprocesses, too
db4702a 
After grpc#28963, the Python entry points run with "python3" found on the
PATH.

It makes sense to use the same for subprocesses, for two reasons:

- There might be no "python" in the PATH, as in my Mac with brew
- Having a different Python for subprocesses, especially if it's a
  Python 2, is asking for trouble.
@copybara-service copybara-service bot mentioned this pull request Nov 23, 2023
Closed
copybara-service bot pushed a commit that referenced this pull request Nov 23, 2023
@lepistone @copybara-github
[benchmark] Use python3 for subprocesses, too (#34940)
6cacea4 
After #28963, the Python entry points run with "python3" found on the
PATH.

It makes sense to use the same for subprocesses, for two reasons:

- There might be no "python" in the PATH, as in my Mac with brew
- Having a different Python for subprocesses, especially if it's a
  Python 2, is asking for trouble.

<!--

If you know who should review your pull request, please assign it to that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the appropriate
lang label.

-->

Closes #34940

COPYBARA_INTEGRATE_REVIEW=#34940 from lepistone:same-python db4702a
FUTURE_COPYBARA_INTEGRATE_REVIEW=#34940 from lepistone:same-python db4702a
PiperOrigin-RevId: 584885685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnossen gnossen gnossen approved these changes

Assignees

@gnossen gnossen

Labels
bloat/none imported Specifies if the PR has been imported to the internal repository perf-change/none release notes: no Indicates if PR should not be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jtattermusch @gnossen