feat: improve dynamo_check.py messaging & instructions #2453
Conversation
keivenchang
requested review from
biswapanda,
hhzhang16,
hutm,
ishandhanani,
julienmancuso,
mohammedabdulwahhab and
nnshah1
as code owners
WalkthroughRefactors deploy/dynamo_check.py: replaces legacy build guidance with development/production build modes, removes CUDA memory clearing, adds maturin detection, overhauls system/GPU info reporting, updates path displays, introduces --build-options CLI flag, removes --clear-cuda-memory, adjusts main flow to show build options or run checks accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as dynamo_check.py (CLI)
participant DC as DynamoChecker
participant Sys as System (nvidia-smi, Python, maturin)
U->>CLI: Invoke with args
CLI->>DC: init / parse args
alt --build-options
DC->>Sys: Detect maturin, env paths
DC-->>U: Print development/production build steps
else normal run
DC->>Sys: Collect system info (maturin, Python, Torch)
DC->>Sys: Query GPUs via nvidia-smi (with fallbacks)
DC-->>U: Print formatted system/GPU info
DC-->>U: Run checks and examples if no failures
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
deploy/dynamo_check.py (3)
755-757: Fix tree drawing: Python should close the System info blockPython is always the last top-level line in System info. Using “├─” based on has_cargo renders an open branch with no following sibling lines in this section.
Apply this diff:
- more_after_python = bool(has_cargo) - print(f"{'├─' if more_after_python else '└─'} {python_line}") + # Python is the last top-level line in System info + print(f"└─ {python_line}")
911-914: Nice touch: PYTHONPATH preview with countThe preview string and count are useful. Consider quoting the variable in the example to avoid surprises with spaces/colons, but current output is still clear.
Example tweak (optional, outside selected lines):
- export PYTHONPATH="...computed..." rather than without quotes.
1539-1564: CLI flow looks solid; minor improvement: DRY the build-options blockThe --import-check-only and the standalone --build-options branches duplicate logic for computing and printing build options. Consider extracting a tiny helper to avoid divergence.
Example (outside this hunk):
- def _maybe_show_build_options(self): compute pythonpath, validate workspace, and call _show_build_options or print the appropriate error.
Then reuse it in both branches.
📜 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)
deploy/dynamo_check.py(15 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 (4)
deploy/dynamo_check.py (4)
610-618: Improve System info ordering consistency and clarity
- You already include the GPU power/memory suffix in the GPU line; keeping CUDA as a dedicated next line is good. Maturin and Python then follow, which looks consistent.
- Minor polish: with the above tree-connector fix, the section renders cleanly.
Also applies to: 725-763
1043-1052: Good: absolute path with $HOME replacement for workspace modulesSwitching to absolute workspace paths improves clarity when multiple environments are active. Timestamps only for generated artifacts is also sound.
1298-1306: Re-check failure criteria after relaxing cargo from fatal to warningOnce cargo absence is no longer fatal (see earlier suggestion), any_failures should reflect import failures and explicit errors only. With the suggested change in _print_system_info, this block is already correct.
You can verify expected exit codes locally:
- No cargo installed, all imports succeed: exit 0
- Runtime okay, framework missing: exit 1 (build options printed)
- Invalid --path: exit 1
1412-1426: Troubleshooting: good CUDA memory tips; keep them explicitly “manual”Clarifying that CUDA cache clearing is manual (not automated) avoids confusion after removing --clear-cuda-memory. This section reads well.
keivenchang
force-pushed
the
keivenchang/refactor__dynamo_check-better-instructions
branch
from
f7e3f94 to
d4d6d6e
Compare
keivenchang
changed the title
…arer output structure
keivenchang
force-pushed
the
keivenchang/refactor__dynamo_check-better-instructions
branch
from
949768d to
f9048fa
Compare
…tting - Add hostname to System info header - Display git SHA (12 chars) and commit timestamp in Dynamo header - Restructure Python section with Torch and PYTHONPATH as child elements - Place Torch info before PYTHONPATH for better readability - Add _get_git_info method to retrieve git commit metadata
keivenchang
deleted the
keivenchang/refactor__dynamo_check-better-instructions
branch
Co-authored-by: Keiven Chang <[email protected]> Signed-off-by: Hannah Zhang <[email protected]>
Co-authored-by: Keiven Chang <[email protected]>
Co-authored-by: Keiven Chang <[email protected]> Signed-off-by: Krishnan Prashanth <[email protected]>
Co-authored-by: Keiven Chang <[email protected]> Signed-off-by: nnshah1 <[email protected]>
Overview:
Add better messaging and checks (like maturin)
Details:
Where should the reviewer start?
Related Issues:
Summary by CodeRabbit