Address unpinned Python dependencies #5661
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the repository's supply chain security by transitioning from unpinned to pinned Python dependencies. By enforcing specific versions and hashes, the changes ensure build reproducibility and mitigate risks associated with dynamic package resolution. Additionally, the move to local vendoring for remote requirements further hardens the build process against external network dependencies. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates dependency management by pinning versions and vendoring requirements files across several components. Feedback focuses on a critical issue where a generated requirements file incorrectly points to a private staging registry, which would cause build failures for external users. Additionally, the transition from dynamic fetching to local vendoring of slurm-gcp requirements was identified as a violation of the project's consistency guidelines, which prefer fetching shared scripts from the master branch of the slurm-gcp repository.
| --index-url https://us-python.pkg.dev/artifact-foundry-prod/ah-3p-staging-python/simple/ | ||
| --extra-index-url https://pypi.org/simple |
There was a problem hiding this comment.
The --index-url points to a staging artifact registry (ah-3p-staging-python). For a community module, this should point to the public PyPI or be omitted to use the default. Using a staging registry will cause build failures for users who do not have access to this specific GCP project. Please re-generate this file using the public PyPI index.
--index-url https://pypi.org/simple
| # --- Install Python Dependencies --- | ||
| RUN pip install --no-cache-dir --upgrade pip setuptools wheel && \ | ||
| pip install --no-cache-dir --prefer-binary --no-build-isolation -r https://raw.githubusercontent.com/GoogleCloudPlatform/slurm-gcp/refs/heads/master/scripts/requirements.txt && \ | ||
| pip install --no-cache-dir --prefer-binary --no-build-isolation -r slurm_gcp_requirements.txt && \ |
There was a problem hiding this comment.
This change replaces the dynamic fetching of requirements from the slurm-gcp master branch with a local vendored file. This violates the general rule: "To maintain consistency across blueprints, shared scripts ... should be fetched from the master branch of the GoogleCloudPlatform/slurm-gcp repository rather than being pinned to a specific version." While pinning improves determinism and security, it breaks consistency with the latest slurm-gcp scripts that the toolkit depends on. Consider if the security benefit outweighs the maintenance burden and risk of drift.
References
- To maintain consistency across blueprints, shared scripts (e.g., sudo-oslogin, imex_prolog, imex_epilog) should be fetched from the master branch of the GoogleCloudPlatform/slurm-gcp repository rather than being pinned to a specific version.
Description
This PR improves the supply chain security of the repository by pinning build-time Python dependencies in Dockerfiles and scripts. This ensures deterministic builds and reduces the risk of installing unexpected package versions.
Changes Made
community/cos-nvidia-bug-reportrequirements.txtwith specific versions and hashes.requirements.infor tracking top-level dependencies.tools/cloud-buildrequirements.txtfor required tools.tools/cloud-workstationsDockerfileto use this local vendored file.Verification Plan
Automated Tests
Manual Verification
Dockerfilecorrectly references the local vendored file.