[TRTLLM-8236][infra] fix platform tag for public wheel - #14616
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional Kitmaker wheel dry-run to the Jenkins L0 build flow and forwards a new plat_name into wheel packaging. Updates sanity-check tuples with a platformTag slot and extends build_wheel.py with a validated --plat-name argument used during python -m build. ChangesKitmaker dry-run + Platform name support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jenkins/L0_Test.groovy (1)
4235-4288:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
platformTagis configured but never consumed in the sanity-check pipeline.The new slot is added in configs, but the job logic still only uses indices up to
values[6];values[7]never reachesrunLLMBuild, so wheel builds ignore the configured platform tag.Suggested fix
- def wheelName = "" + def wheelName = "" + def platformTag = values[7] def cpver = "cp312" def pyver = "3.12" @@ buildRunner("[${toStageName(values[1], key)}] Build") { - wheelName = runLLMBuild(pipeline, cpu_arch, values[3], "", versionLocal, cpver) + wheelName = runLLMBuild(pipeline, cpu_arch, values[3], "", versionLocal, cpver, platformTag) }And update
runLLMBuild(...)to pass it through:-def runLLMBuild(pipeline, cpu_arch, reinstall_dependencies=false, wheel_path="", version_local="", cpver="cp312") +def runLLMBuild(pipeline, cpu_arch, reinstall_dependencies=false, wheel_path="", version_local="", cpver="cp312", platformTag="linux") @@ - withCredentials([usernamePassword(credentialsId: "urm-artifactory-creds", usernameVariable: 'CONAN_LOGIN_USERNAME', passwordVariable: 'CONAN_PASSWORD')]) { - trtllm_utils.llmExecStepWithRetry(pipeline, script: "#!/bin/bash \n" + "cd tensorrt_llm/ && python3 scripts/build_wheel.py --use_ccache -G Ninja -j ${BUILD_JOBS} -D 'WARNING_IS_ERROR=ON' ${buildArgs}") + withCredentials([usernamePassword(credentialsId: "urm-artifactory-creds", usernameVariable: 'CONAN_LOGIN_USERNAME', passwordVariable: 'CONAN_PASSWORD')]) { + trtllm_utils.llmExecStepWithRetry(pipeline, script: "#!/bin/bash \n" + "cd tensorrt_llm/ && python3 scripts/build_wheel.py --use_ccache -G Ninja -j ${BUILD_JOBS} -D 'WARNING_IS_ERROR=ON' --platform-tag ${platformTag} ${buildArgs}") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jenkins/L0_Test.groovy` around lines 4235 - 4288, The configs add a new eighth slot (platformTag) but the pipeline still reads only up to values[6], so values[7] is never passed into the build and wheel logic; update the code that unpacks config arrays (the spot using values[0]..values[6]) to also extract values[7] as platformTag and thread that platformTag through to any downstream calls (notably the runLLMBuild invocation) so runLLMBuild receives and uses the platformTag parameter; also update runLLMBuild's signature/parameters to accept platformTag and propagate it into the wheel/build steps where platform-specific behavior is applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build_wheel.py`:
- Around line 1105-1107: The platform tag (--plat-name) is only added to
extra_wheel_build_args once, so the second python -m build invocation doesn't
receive it; ensure both wheel build calls receive the same plat override by
appending the f' --config-setting="--plat-name={platform_tag}_{arch}"' to
extra_wheel_build_args before any build is executed (or explicitly pass that
string into both build invocations), referencing the existing variables
platform_tag, arch, and extra_wheel_build_args and the two python -m build calls
so both produce the same deterministic wheel filename.
- Around line 1311-1314: The --platform-tag value is interpolated into a shell
command unsafely (from parser.add_argument into a shell=True call); validate or
sanitize platform_tag before using it in any shell invocation by restricting it
to a safe pattern or a whitelist (e.g. allowed values like "linux",
"manylinux_2_28" or a regex such as ^[A-Za-z0-9_\\-]+$), and reject/raise
argparse.ArgumentTypeError for invalid values (or better: avoid shell=True and
build the command as a list). Locate the platform_tag variable from
parser.add_argument and add a validator function (passed as the type in
add_argument or run immediately after parsing) that enforces the whitelist/regex
and returns the sanitized value before any subprocess.run/call where
platform_tag is interpolated.
---
Outside diff comments:
In `@jenkins/L0_Test.groovy`:
- Around line 4235-4288: The configs add a new eighth slot (platformTag) but the
pipeline still reads only up to values[6], so values[7] is never passed into the
build and wheel logic; update the code that unpacks config arrays (the spot
using values[0]..values[6]) to also extract values[7] as platformTag and thread
that platformTag through to any downstream calls (notably the runLLMBuild
invocation) so runLLMBuild receives and uses the platformTag parameter; also
update runLLMBuild's signature/parameters to accept platformTag and propagate it
into the wheel/build steps where platform-specific behavior is applied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4e9e6bad-7c16-4278-8202-2fc3f0d57554
📒 Files selected for processing (2)
jenkins/L0_Test.groovyscripts/build_wheel.py
a7848bc to
5ff7502
Compare
|
/bot run --skip-test |
|
PR_Github #50454 [ run ] triggered by Bot. Commit: |
|
PR_Github #50454 [ run ] completed with state
|
|
PR_Github #50486 [ run ] triggered by Bot. Commit: |
|
PR_Github #50486 [ run ] completed with state
|
|
PR_Github #50499 [ run ] triggered by Bot. Commit: |
|
PR_Github #50520 [ run ] triggered by Bot. Commit: |
|
PR_Github #50499 [ run ] completed with state |
|
PR_Github #50520 [ run ] completed with state |
|
PR_Github #50560 [ run ] triggered by Bot. Commit: |
|
PR_Github #50560 [ run ] completed with state
|
|
PR_Github #50567 [ run ] triggered by Bot. Commit: |
|
PR_Github #50567 [ run ] completed with state |
|
PR_Github #50706 [ run ] triggered by Bot. Commit: |
|
PR_Github #50706 [ run ] completed with state
|
|
PR_Github #50707 [ run ] triggered by Bot. Commit: |
|
PR_Github #51097 [ run ] completed with state
|
|
PR_Github #51134 [ run ] triggered by Bot. Commit: |
|
PR_Github #51134 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51178 [ run ] triggered by Bot. Commit: |
|
PR_Github #51178 [ run ] completed with state
|
|
PR_Github #51200 [ run ] triggered by Bot. Commit: |
|
PR_Github #51200 [ run ] completed with state
|
|
PR_Github #51228 [ run ] triggered by Bot. Commit: |
|
PR_Github #51228 [ run ] completed with state |
|
/bot run --skip-test --disable-fail-fast |
|
PR_Github #51469 [ run ] triggered by Bot. Commit: |
|
PR_Github #51469 [ run ] completed with state |
Signed-off-by: Yiteng Niu <6831097+niukuo@users.noreply.github.com>
|
/bot run --skip-test --disable-fail-fast |
|
PR_Github #51966 [ run ] triggered by Bot. Commit: |
|
PR_Github #51966 [ run ] completed with state |
|
/bot skip --comment "sanity check passed" |
|
PR_Github #52011 [ skip ] triggered by Bot. Commit: |
|
PR_Github #52011 [ skip ] completed with state |
Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
3 participants
Summary by CodeRabbit
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.