Skip to content

Conversation

@saturley-hall
Copy link
Member

@saturley-hall saturley-hall commented Sep 4, 2025

Overview:

Installs NIXL wheel to build stage system-wide python environment so it is available in the dev stage as well.

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

Closes OPS-877

Summary by CodeRabbit

  • Chores

    • Updated container build to install the NIXL wheel using both uv pip and standard pip within the same step, ensuring it’s present in both environments.
  • Bug Fixes

    • Improves container reliability by making the NIXL package consistently available at runtime across environments, reducing potential import issues.
  • Build

    • No changes to build stages or logic beyond the dual installation approach.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 4, 2025

Walkthrough

Adds a second installation step for the nixl wheel in container/Dockerfile.trtllm: after installing via uv pip, it now also installs via system pip in the same RUN command using &&. No other stages or logic are modified.

Changes

Cohort / File(s) Summary
Docker build - nixl installation
container/Dockerfile.trtllm
In the nixl wheel install step, chains uv pip install /workspace/wheels/nixl/*.whl with && pip install /workspace/wheels/nixl/*.whl to install into both uv and system environments within a single RUN. No other Dockerfile changes.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Docker as Docker Build
    participant RUN as RUN Layer
    participant UV as uv pip
    participant PIP as system pip

    Docker->>RUN: Execute nixl install step
    RUN->>UV: uv pip install /workspace/wheels/nixl/*.whl
    alt uv install succeeds
        RUN->>PIP: pip install /workspace/wheels/nixl/*.whl
        note right of PIP: Installs nixl into system environment
        RUN-->>Docker: Layer completes
    else uv install fails
        note over RUN: Second pip install is not executed due to &&
        RUN-->>Docker: Build fails at this step
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

In a crate of wheels I hop with glee,
uv first, then pip for me.
Two little installs, one tidy run,
layers stack—job neatly done.
Thump goes the log, success we seek,
carrots cached for build next week. 🥕🐇


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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 0

🧹 Nitpick comments (2)
container/Dockerfile.trtllm (2)

231-232: Be explicit about target interpreters to avoid ambiguity; prefer python -m pip.

Make it deterministic that uv installs into the created venv and system install uses the right interpreter.

Apply:

-    uv pip install /workspace/wheels/nixl/*.whl && \
-    pip install /workspace/wheels/nixl/*.whl
+    uv pip install --python "$VIRTUAL_ENV/bin/python" /workspace/wheels/nixl/*.whl && \
+    python3 -m pip install --no-cache-dir /workspace/wheels/nixl/*.whl

231-232: Optional: Move install to wheel_builder or dev stage as hinted by the TODO.

Keeps build stage lean and reduces duplicate installs across stages; copy the wheel and install once where it’s consumed.

I can draft a follow-up patch to relocate the install and wire the wheel into dev/runtime via the existing wheelhouse.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c403d18 and 8afba0b.

📒 Files selected for processing (1)
  • container/Dockerfile.trtllm (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). (1)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
container/Dockerfile.trtllm (1)

231-232: LGTM: NIXL now installed into both uv venv and system env, matching PR objective.

This should make nixl importable in the dev stage without an extra step.

@saturley-hall saturley-hall merged commit 1fc4050 into main Sep 4, 2025
11 checks passed
@saturley-hall saturley-hall deleted the harrison/fix_trtllm_nixl_NVBug_5497851 branch September 4, 2025 20:47
saturley-hall added a commit that referenced this pull request Sep 4, 2025
saturley-hall added a commit that referenced this pull request Sep 4, 2025
dillon-cullinan pushed a commit that referenced this pull request Sep 5, 2025
nnshah1 pushed a commit that referenced this pull request Sep 8, 2025
Signed-off-by: Harrison King Saturley-Hall <[email protected]>
Signed-off-by: nnshah1 <[email protected]>
rmccorm4 added a commit that referenced this pull request Sep 9, 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.

3 participants