Don't fail if creating a venv didn't succeed. Also provide a way to disable creating separate venvs. #26753
Conversation
…able creating a venv.
tvalentyn
changed the title
|
r: @liferoad |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Codecov Report
@@ Coverage Diff @@
## master #26753 +/- ##
==========================================
- Coverage 72.05% 71.97% -0.08%
==========================================
Files 745 747 +2
Lines 101204 101306 +102
==========================================
- Hits 72921 72916 -5
- Misses 26824 26931 +107
Partials 1459 1459
Flags with carried forward coverage won't be shown. Click here to find out more. see 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
cc: @riteshghorse |
sdks/python/container/boot.go
Outdated
| if os.Getenv("RUN_PYTHON_SDK_IN_DEFAULT_ENVIRONMENT") == "" { | ||
| venvDir, err := setupVenv(ctx, logger, "/opt/apache/beam-venv", *id) | ||
| if err != nil { | ||
| logger.Printf(ctx, "Using default environment, since creating a virtual environment for the SDK harness didn't succeed: "+err.Error()) |
There was a problem hiding this comment.
If my Go knowledge is correct, this will be still logged as INFO?
There was a problem hiding this comment.
As implemented, that's correct. The logger is just a lightweight wrapper to actually send it through the logging service instead of it hopefully getting picked up via STDOut + Err (with those being a fallback). Info logs currently, since it doesn't seem valuable to differentiate at this time.
There was a problem hiding this comment.
Given this error is not critical, Info level is appropriate.
|
R: @lostluck |
sdks/python/container/boot.go
Outdated
| if os.Getenv("RUN_PYTHON_SDK_IN_DEFAULT_ENVIRONMENT") == "" { | ||
| venvDir, err := setupVenv(ctx, logger, "/opt/apache/beam-venv", *id) | ||
| if err != nil { | ||
| logger.Printf(ctx, "Using default environment, since creating a virtual environment for the SDK harness didn't succeed: "+err.Error()) |
There was a problem hiding this comment.
As implemented, that's correct. The logger is just a lightweight wrapper to actually send it through the logging service instead of it hopefully getting picked up via STDOut + Err (with those being a fallback). Info logs currently, since it doesn't seem valuable to differentiate at this time.
Co-authored-by: Robert Burke <lostluck@users.noreply.github.com>
|
Thank you very much, all. |
|
cc: @phoerious FYI. |
|
Run PythonDocker PreCommit |
|
Run Python_PVR_Flink PreCommit |
…isable creating separate venvs. (apache#26753) Co-authored-by: Robert Burke <lostluck@users.noreply.github.com>
#16658 made a change to Python SDK harness container boot sequence to launch SDK processes in separately created virtual environments.
It appears that the venv dependency is sometimes not available on non-beam Python container images. Users who supply custom containers may run into errors when
python3-venvis not installed, and need to install it separately, which is inconvenient.Creating a venv is not strictly required, therefore we can fallback on prior behavior to use global environment.
Drive-by changes:
ENV RUN_PYTHON_SDK_IN_DEFAULT_ENVIRONMENT=1Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.