Add additional ARGS for pylock locations and names #2713
Conversation
github-actions
bot
added
the
review-requested
WalkthroughAdds build arguments Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
jupyter/minimal/ubi9-python-3.12/Dockerfile.cpu (1)
5-6: Consider adding default values for safety.The ARG declarations enable the parameterization correctly. However, consider adding default values to prevent build failures if the build-args are not passed:
-ARG PYLOCK_PATH -ARG PYLOCK_NAME +ARG PYLOCK_PATH=/pylock.toml +ARG PYLOCK_NAME=pylock.tomljupyter/minimal/ubi9-python-3.12/Dockerfile.cuda (1)
7-8: Consider adding default values for safety.Same recommendation as Dockerfile.cpu: add default values to ARGs to prevent build failures if build-args are not provided.
-ARG PYLOCK_PATH -ARG PYLOCK_NAME +ARG PYLOCK_PATH=/pylock.toml +ARG PYLOCK_NAME=pylock.tomljupyter/minimal/ubi9-python-3.12/Dockerfile.rocm (1)
5-6: Consider adding default values for safety.Same recommendation as the other Dockerfiles: add default values to prevent build failures if build-args are not provided.
-ARG PYLOCK_PATH -ARG PYLOCK_NAME +ARG PYLOCK_PATH=/pylock.toml +ARG PYLOCK_NAME=pylock.toml
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
jupyter/minimal/ubi9-python-3.12/Dockerfile.cpu(3 hunks)jupyter/minimal/ubi9-python-3.12/Dockerfile.cuda(3 hunks)jupyter/minimal/ubi9-python-3.12/Dockerfile.rocm(3 hunks)jupyter/minimal/ubi9-python-3.12/build-args/cpu.conf(1 hunks)jupyter/minimal/ubi9-python-3.12/build-args/cuda.conf(1 hunks)jupyter/minimal/ubi9-python-3.12/build-args/rocm.conf(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (cuda-jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-minimal-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (rocm-jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/s390x, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/ppc64le, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
🔇 Additional comments (12)
jupyter/minimal/ubi9-python-3.12/build-args/rocm.conf (1)
2-3: LGTM! Consistent parameterization across flavors.The PYLOCK_PATH and PYLOCK_NAME configuration is consistent with the cpu.conf and cuda.conf variants, enabling future differentiation of pylock files for konflux builds while maintaining current behavior.
jupyter/minimal/ubi9-python-3.12/build-args/cpu.conf (1)
3-4: LGTM! Consistent configuration.The build arguments are correctly configured and consistent with the other flavor configurations.
jupyter/minimal/ubi9-python-3.12/build-args/cuda.conf (1)
2-3: LGTM! Configuration is consistent.Build arguments match the pattern established in cpu.conf and rocm.conf.
jupyter/minimal/ubi9-python-3.12/Dockerfile.cpu (3)
71-72: LGTM! Correct ARG re-declaration in stage.The ARGs are correctly re-declared in the jupyter-minimal stage to make them available after the FROM instruction.
107-107: LGTM! Install command correctly uses dynamic pylock reference.The pip install command properly references the pylock file via
${PYLOCK_NAME}, maintaining consistency with the parameterized COPY command.
98-99: Verification confirmed: pylock.toml file exists at expected location.The parameterized path concatenation
${MINIMAL_SOURCE_CODE}${PYLOCK_PATH}correctly constructsjupyter/minimal/ubi9-python-3.12/pylock.toml(413K, verified Nov 24 13:34). The COPY commands will execute successfully.jupyter/minimal/ubi9-python-3.12/Dockerfile.cuda (3)
70-71: LGTM! Correct stage-level ARG declarations.
97-98: LGTM! Parameterized COPY implementation is consistent.The dynamic pylock handling matches the pattern in Dockerfile.cpu.
106-106: LGTM! Install command correctly parameterized.jupyter/minimal/ubi9-python-3.12/Dockerfile.rocm (3)
68-69: LGTM! Correct ARG re-declarations.
95-96: LGTM! Consistent parameterized implementation.The COPY commands match the pattern established in the cpu and cuda variants.
104-104: LGTM! Install command correctly parameterized.
atheo89
force-pushed
the
RHAIENG-2096
branch
from
5d55ea5 to
7a03ce2
Compare
atheo89
force-pushed
the
RHAIENG-2096
branch
from
7a03ce2 to
cc3e106
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jiridanek The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
makes sense, should work |
|
@atheo89 do we close this PR, given the apicc phase 2 postponement, or do you think it is still better to go ahead with it and merge? I'm fine either way, although I slightly prefer closing. |
|
@atheo89: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Ok, we will reopen it once we re-initialize this work. |
3 participants
This PR prepares the ground for: https://issues.redhat.com/browse/RHAIENG-1650 as i need to specify different locations for pylocks on the konflux flavors that introduced on red-hat-data-services#1668
Description
Add additional ARGS for pylock locations and names
Related to: https://issues.redhat.com/browse/RHAIENG-2096
How Has This Been Tested?
local
gmake cuda-jupyter-minimal-ubi9-python-3.12completed successfully (for all the flavors cpu/cuda/rocm).Self checklist (all need to be checked):
make test(gmakeon macOS) before asking for reviewDockerfile.konfluxfiles should be done inodh/notebooksand automatically synced torhds/notebooks. For Konflux-specific changes, modifyDockerfile.konfluxfiles directly inrhds/notebooksas these require special attention in the downstream repository and flow to the upcoming RHOAI release.Merge criteria:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.