-
Notifications
You must be signed in to change notification settings - Fork 627
fix: Fix the Dockerfile name #2468
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
WalkthroughUpdates the trtllm path filter in .github/workflows/trigger_ci.yml from container/Dockerfile.tensorrt_llm to container/Dockerfile.trtllm so that changes to the new Dockerfile trigger src_changes.trtllm and RUN_TRTLLM. No other workflow logic or filters were modified. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant WF as trigger_ci.yml
participant PIPE as Trigger Pipeline
Dev->>GH: Push/PR with file changes
GH->>WF: Evaluate path filters
alt Dockerfile.trtllm changed
WF->>WF: Set src_changes.trtllm = true
WF->>PIPE: Enable RUN_TRTLLM
else No relevant change
WF->>PIPE: RUN_TRTLLM not enabled
end
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/trigger_ci.yml (1)
55-61
: Consider temporarily including the legacy Dockerfile path for forward/backward compatibilityIf any long-lived branches or pending PRs still reference the old Dockerfile.tensorrt_llm, CI won’t trigger for TRT LLM changes on those branches. You can include both paths short-term to avoid blind spots.
Apply this minimal additive change:
trtllm: + - 'container/Dockerfile.tensorrt_llm' - 'container/Dockerfile.trtllm' - 'components/backends/trtllm/**' - 'container/build.sh' - 'container/build_trtllm_wheel.sh' - 'container/deps/**' - 'tests/serve/test_trtllm.py'
📜 Review details
Configuration used: .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.
📒 Files selected for processing (1)
.github/workflows/trigger_ci.yml
(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 (2)
.github/workflows/trigger_ci.yml (2)
55-55
: Path filter rename looks correct and aligns with PR intentSwitching to 'container/Dockerfile.trtllm' matches the stated Dockerfile rename and will properly flip src_changes.trtllm for RUN_TRTLLM.
55-61
: ✅ New TRT LLM Dockerfile present & no legacy references found
All checks passed:
container/Dockerfile.trtllm
(and its prebuilt variant) exists- No occurrences of
Dockerfile.tensorrt_llm
remain in the repo or workflowsNo further action needed.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Overview:
Missed renaming the dockerfile here
Summary by CodeRabbit