-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Hardware][IBM Z] Fix Outlines Core issue for s390x #24034
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
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.
Code Review
This pull request aims to fix outlines-core
for the s390x architecture by building it from source with necessary patches. The overall approach is sound, but the implementation uses sed
commands with hardcoded line numbers and string replacements to patch dependencies in the Dockerfile. This is a fragile practice that can easily break with upstream changes, making the build process difficult to maintain. My review includes suggestions to replace these sed
commands with more robust patch files to improve the long-term stability of the build.
docker/Dockerfile.s390x
Outdated
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.
Using sed
with hardcoded line numbers to patch source files is highly fragile. If the upstream file aws-lc/crypto/pem/pem_lib.c
is modified, these line numbers may become incorrect, which could cause the build to fail or apply the patch to the wrong lines. A more robust and maintainable approach is to use a patch file.
I recommend creating a patch file (e.g., aws-lc-s390x.patch
) with the required changes and applying it using git apply
or patch
. This would make the build process more resilient to upstream changes.
docker/Dockerfile.s390x
Outdated
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.
Using sed
and echo
to modify Cargo.toml
is fragile and can lead to build failures.
- The
sed
command on line 237 is sensitive to the exact formatting of the version string. - The
echo
commands on lines 238-239 could create a duplicate[patch.crates-io]
section if one already exists, resulting in an invalid TOML file.
A more robust solution is to use a patch file for these modifications. This ensures the changes are applied atomically and are not dependent on the exact formatting of the original file.
#22725 (comment) |
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
Signed-off-by: Rehan Khan <[email protected]>
9e465b0
to
34abd8b
Compare
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.
LGTM
@youkaichao Hi, could you merge this pr |
@JaheimLee @youkaichao can you please help adding the ready label so all CI jobs starts running. |
I don't have access rights. Maybe you need to add other reviewers who have bandwidth. |
Signed-off-by: Rehan Khan <[email protected]>
Signed-off-by: Rehan Khan <[email protected]>
Signed-off-by: Rehan Khan <[email protected]>
Signed-off-by: Rehan Khan <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Purpose
Fix outlines-core for s390x
Test Plan
Inferencing is working as expected, tested locally.
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.