Skip to content

Conversation

@nv-kmcgill53
Copy link
Contributor

@nv-kmcgill53 nv-kmcgill53 commented Nov 17, 2025

Overview:

This fixes a bug in build.sh where, when building TRTLLM from source for installation in the container, the TENSORRTLLM_PIP_WHEEL_DIR was not set to a default when omitted as a flag on the command line. This caused a build failure after the trtllm wheel is built when a null directory was trying to be constructed.

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

Summary by CodeRabbit

  • Chores
    • Improved build process reliability by ensuring the wheel output directory is properly initialized with a default value and displayed during the build process for better transparency.

@nv-kmcgill53 nv-kmcgill53 requested review from a team as code owners November 17, 2025 22:54
@nv-kmcgill53 nv-kmcgill53 changed the title Fixing bug where TENSORRTLLM_PIP_WHEEL_DIR not being set. fix: TENSORRTLLM_PIP_WHEEL_DIR has reasonable default set when building and installing TRTLLM from source Nov 17, 2025
@github-actions github-actions bot added the fix label Nov 17, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 17, 2025

Walkthrough

The build script now defaults the TENSORRTLLM_PIP_WHEEL_DIR variable to a predetermined value when not explicitly provided and adds an echo statement to display the resolved output directory path during execution.

Changes

Cohort / File(s) Summary
Variable Default and Output Directory Display
container/build.sh
Added default assignment for TENSORRTLLM_PIP_WHEEL_DIR using parameter expansion and diagnostic echo statement to print the resolved directory path

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰✨ A wheel path now clear, with defaults so dear,
No mystery, just echoes to hear!
Two lines, one goal—let builders see true,
The output directory, polished and new! 🛠️

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the overview of the bug fix and references a related issue, though the Details and Where should the reviewer start sections are empty.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title accurately describes the main change: setting a reasonable default for TENSORRTLLM_PIP_WHEEL_DIR when building TRTLLM from source, which directly addresses the bug fix implemented in the PR.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between adbe133 and e1c0d57.

📒 Files selected for processing (1)
  • container/build.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 2489
File: container/deps/vllm/install_vllm.sh:151-152
Timestamp: 2025-08-18T16:52:15.659Z
Learning: The VLLM_PRECOMPILED_WHEEL_LOCATION environment variable, when exported, automatically triggers vLLM's build system to use the precompiled wheel instead of building from source, even when using standard `uv pip install .` commands in container/deps/vllm/install_vllm.sh.
Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 2489
File: container/deps/vllm/install_vllm.sh:151-152
Timestamp: 2025-08-18T16:52:15.659Z
Learning: The VLLM_PRECOMPILED_WHEEL_LOCATION environment variable is an official vLLM environment variable that, when exported, automatically triggers vLLM's build system to use the specified precompiled wheel instead of building from source. This works even with standard `uv pip install .` commands without requiring explicit reference to the variable in the install command. The vLLM build system internally detects and uses this environment variable.
📚 Learning: 2025-08-18T16:52:15.659Z
Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 2489
File: container/deps/vllm/install_vllm.sh:151-152
Timestamp: 2025-08-18T16:52:15.659Z
Learning: The VLLM_PRECOMPILED_WHEEL_LOCATION environment variable, when exported, automatically triggers vLLM's build system to use the precompiled wheel instead of building from source, even when using standard `uv pip install .` commands in container/deps/vllm/install_vllm.sh.

Applied to files:

  • container/build.sh
⏰ 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)
  • GitHub Check: operator (arm64)
  • GitHub Check: operator (amd64)
  • GitHub Check: trtllm (arm64)
  • GitHub Check: sglang (arm64)
  • GitHub Check: sglang (amd64)
  • GitHub Check: vllm (arm64)
  • GitHub Check: vllm (amd64)
  • GitHub Check: trtllm (amd64)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
container/build.sh (1)

752-766: Verify the build context path and wheel discovery logic after applying the default.

Once DEFAULT_TENSORRTLLM_PIP_WHEEL_DIR is properly defined, ensure that the build script correctly discovers the wheel file at line 765 using find $TENSORRTLLM_PIP_WHEEL_DIR. Confirm that the default directory is both writable and that subsequent build steps handle the case where the directory exists but is empty.

@nv-kmcgill53 nv-kmcgill53 merged commit 771d8c0 into main Nov 18, 2025
25 of 27 checks passed
@nv-kmcgill53 nv-kmcgill53 deleted the kmcgill/build-sh-trtllm-bug branch November 18, 2025 15:49
nv-kmcgill53 added a commit that referenced this pull request Nov 18, 2025
assert "Intent to Download TRTLLM: false" in stdout
assert "Intent to Install TRTLLM: false" in stdout
assert "Intent to Build TRTLLM: true" in stdout
assert "TRTLLM pip wheel output directory is: /tmp/trtllm_wheel/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this assert is always true, are we just want to print log message here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, this was a copy paste error

nv-tusharma pushed a commit that referenced this pull request Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants