lostluck commented on code in PR #26753:
URL: https://github.com/apache/beam/pull/26753#discussion_r1198049182
##########
sdks/python/container/boot.go:
##########
@@ -157,15 +156,20 @@ func launchSDKProcess() error {
signalChannel := make(chan os.Signal, 1)
signal.Notify(signalChannel, syscall.SIGHUP, syscall.SIGINT,
syscall.SIGTERM)
- venvDir, err := setupVenv(ctx, logger, "/opt/apache/beam-venv", *id)
- if err != nil {
- return errors.New("failed to initialize Python venv")
- }
- cleanupFunc := func() {
- os.RemoveAll(venvDir)
- logger.Printf(ctx, "Cleaned up temporary venv for worker %v.",
*id)
+ // Create a separate virtual envirionment (with access to globally
installed packages), unless disabled by the user.
+ // This improves usability on runners that persit the execution
environment for the boot entrypoint between multiple pipeline executions.
+ 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())
Review Comment:
```suggestion
logger.Printf(ctx, "Using default environment, since
creating a virtual environment for the SDK harness didn't succeed: %v", err)
```
##########
sdks/python/container/boot.go:
##########
@@ -308,9 +312,8 @@ func StartCommandEnv(env map[string]string, prog string,
args ...string) *exec.C
// setupVenv initializes a local Python venv and sets the corresponding env
variables
func setupVenv(ctx context.Context, logger *tools.Logger, baseDir, workerId
string) (string, error) {
- logger.Printf(ctx, "Initializing temporary Python venv ...")
-
dir := filepath.Join(baseDir, "beam-venv-worker-"+workerId)
+ logger.Printf(ctx, "Initializing temporary Python venv in "+dir)
Review Comment:
```suggestion
logger.Printf(ctx, "Initializing temporary Python venv in %v", dir)
```
##########
sdks/python/container/boot.go:
##########
@@ -157,15 +156,20 @@ func launchSDKProcess() error {
signalChannel := make(chan os.Signal, 1)
signal.Notify(signalChannel, syscall.SIGHUP, syscall.SIGINT,
syscall.SIGTERM)
- venvDir, err := setupVenv(ctx, logger, "/opt/apache/beam-venv", *id)
- if err != nil {
- return errors.New("failed to initialize Python venv")
- }
- cleanupFunc := func() {
- os.RemoveAll(venvDir)
- logger.Printf(ctx, "Cleaned up temporary venv for worker %v.",
*id)
+ // Create a separate virtual envirionment (with access to globally
installed packages), unless disabled by the user.
+ // This improves usability on runners that persit the execution
environment for the boot entrypoint between multiple pipeline executions.
+ 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())
Review 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.
##########
sdks/python/container/boot.go:
##########
@@ -157,15 +156,20 @@ func launchSDKProcess() error {
signalChannel := make(chan os.Signal, 1)
signal.Notify(signalChannel, syscall.SIGHUP, syscall.SIGINT,
syscall.SIGTERM)
- venvDir, err := setupVenv(ctx, logger, "/opt/apache/beam-venv", *id)
- if err != nil {
- return errors.New("failed to initialize Python venv")
- }
- cleanupFunc := func() {
- os.RemoveAll(venvDir)
- logger.Printf(ctx, "Cleaned up temporary venv for worker %v.",
*id)
+ // Create a separate virtual envirionment (with access to globally
installed packages), unless disabled by the user.
+ // This improves usability on runners that persit the execution
environment for the boot entrypoint between multiple pipeline executions.
Review Comment:
```suggestion
// Create a separate virtual environment (with access to globally
installed packages), unless disabled by the user.
// This improves usability on runners that persist the execution
environment for the boot entrypoint between multiple pipeline executions.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
