-
Notifications
You must be signed in to change notification settings - Fork 20
Add konflux Dockerfiles for LLMCompressor image #1510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR introduces two new Dockerfiles for the llmCompressor image with Konflux support. The Dockerfiles are well-structured and follow a multi-stage build pattern, which is great for keeping the final images small.
🔍 General Feedback
- Redundancy: The two new Dockerfiles,
jupyter/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.konflux.cuda
andruntimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.konflux.cuda
, are very similar. To improve maintainability and reduce code duplication, consider creating a common base image that both Dockerfiles can build upon. This would centralize the common setup steps and make future updates easier. - Hardcoded Versions: Several package versions are hardcoded in the Dockerfiles. Using build arguments (
ARG
) for these versions would make the images more flexible and easier to maintain. - Security: It's recommended to verify the checksums of downloaded artifacts to ensure their integrity.
Overall, the changes look good, but addressing the feedback above would improve the quality and maintainability of the Dockerfiles.
Hey @riprasad this PR introduce the llmcompressor image for the upcoming release could you please take a look once you have the chance? There is also on the DevOps side this jira: https://issues.redhat.com/browse/RHOAIENG-32230 that tracks the tekton pipelines for the builds, I linked it here for your reference. Please let us know if anything else is needed. |
/lgtm |
@atheo89: The following test 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. |
Incorporation to Operator: opendatahub-io/opendatahub-operator#2372 |
Add konflux Dockerfiles for llmCompressor image this will be introduced on rhoai2.25
related to: https://issues.redhat.com/browse/RHAIENG-332