[tarun-google]

DOC: https://docs.google.com/document/d/1gNfjbIYNzbv15-RjCj0ad4MHenVPoSjUEcH0Ok3iNyU/edit?usp=sharing

Tested dependencies are downloaded to separate directories by sdk container

requirements.txt are appended with sha-256 to avoid replacing

[github-actions]

📋 Review Summary

This PR fixes artifact staging for Dataflow jobs using prebuilt SDK container images (--prebuild_sdk_container_engine) by decoupling pipeline environment artifact dependencies from the job-level staged resources in apiclient.py, and introduces a fallback in boot.go to copy pre-staged artifacts from /opt/apache/beam/artifacts when no artifacts are provided via the Provision API.

🔍 General Feedback

• The separation of dependencies (for pipeline proto environments) and resources (for Dataflow job staging) in apiclient.py cleanly resolves the regression where Stager.create_job_resources was skipped during job submission.
• However, the fallback in boot.go has a couple of edge cases regarding submission_environment_dependencies.txt (which is always staged by apiclient.py even with prebuilt containers) and potential filename collisions across multiple prebuilt image layers.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

💬 Inline Findings

• sdks/python/container/boot.go (line 270): 🟠 High: len(info.GetDependencies()) == 0 will evaluate to false on Dataflow even when --prebuild_sdk_container_engine is used, preventing this fallback from ever executing on Dataflow. In apiclient.py, _stage_resources calls Stager.create_job_resources with the default log_submission_env_dependencies=True, which stages submission_environment_dependencies.txt to GCS. Dataflow's runner harness populates ProvisionInfo.dependencies from the job's GCS packages list (not from the pipeline proto's Environment.dependencies), meaning info.GetDependencies() will contain submission_environment_dependencies.txt (length 1) rather than being empty.

Additionally, even if /opt/apache/beam/artifacts is copied to dir, installSetupPackages only iterates over the files slice populated from artifacts (lines 296-304). Any workflow.tar.gz or PICKLED_MAIN_SESSION copied from /opt/apache/beam/artifacts won't be in files and thus won't be processed by installSetupPackages (though PICKLED_MAIN_SESSION in dir is still picked up by sdk_worker_main.py). Consider checking if /opt/apache/beam/artifacts exists and merging/copying any local artifacts not already present in dir.

• sdks/python/container/boot.go (line 484): 🟡 Medium: copyDir walks src recursively (which includes subdirectories like /opt/apache/beam/artifacts/<hash>/... created by boot --setup_only during the image build), but flattens all files into the top-level dst directory using filepath.Base(path). Because boot --setup_only already copies artifacts from /opt/apache/beam/artifacts/<name> into /opt/apache/beam/artifacts/<hash>/<name>, every artifact exists at least twice under /opt/apache/beam/artifacts. If a user builds a multi-layered prebuilt image, older subdirectories could overwrite newer top-level files depending on lexical walk order, or if any subdirectory exists, os.Open(path) followed by io.Copy on a directory entry will fail or produce unexpected behavior if info.IsDir() wasn't skipped (though info.IsDir() is skipped, nested files in subdirectories will still collide on filepath.Base(path)). Consider using os.ReadDir(src) instead of filepath.Walk to only copy regular files in the top-level /opt/apache/beam/artifacts directory (and skip artifacts_info.json).

• sdks/python/container/boot.go (line 500): 🟢 Low: defer out.Close() ignores any write/flush errors that occur when closing the destination file, and keeping defer inside the filepath.Walk callback holds all file descriptors open until copyDir returns. Close in and out explicitly per iteration and check the error from out.Close().

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.44%. Comparing base (a8d6afd) to head (49877e4).
⚠️ Report is 35 commits behind head on master.

Files with missing lines	Patch %	Lines
sdks/python/container/boot.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #40699       +/-   ##
===========================================
- Coverage   80.04%   56.44%   -23.61%     
===========================================
  Files         490      871      +381     
  Lines       82916   154214    +71298     
===========================================
+ Hits        66373    87048    +20675     
- Misses      16543    65307    +48764     
- Partials        0     1859     +1859     

Flag	Coverage Δ
go	28.90% <0.00%> (?)
python	80.07% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Assigning reviewers:

R: @claudevdm for label python.
Backup reviewer: shunping (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

[claudevdm]

Can you trigger post commits?

[claudevdm]

R: @tvalentyn can you PTAL too since this has a large blast radius.

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

[tarun-google]

Run Python PostCommit