Skip to content

Fix debugger server failing to detect editable-installed modelopt#1270

Merged
cjluo-nv merged 1 commit intomainfrom
chenjiel/fix_server
Apr 16, 2026
Merged

Fix debugger server failing to detect editable-installed modelopt#1270
cjluo-nv merged 1 commit intomainfrom
chenjiel/fix_server

Conversation

@cjluo-nv
Copy link
Copy Markdown
Collaborator

@cjluo-nv cjluo-nv commented Apr 16, 2026

Summary

  • Removed PYTHONPATH="" python -I override in check_modelopt_local() so the PYTHONPATH validation uses the actual environment instead of an isolated one
  • Moved the workdir log line earlier in server.sh for better debugging visibility

Test plan

  • Start server.sh inside a Docker container and verify it correctly detects editable-installed modelopt
  • Confirm the workdir is logged before the modelopt check runs

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Server startup now displays the configured work directory earlier in the initialization process, providing improved visibility of the active directory during server launch.
    • Simplified the modelopt validation check during server initialization while maintaining the same validation behavior.

Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 16, 2026

📝 Walkthrough

Walkthrough

The server startup sequence is modified to print the configured work directory earlier in the initialization process, immediately after directory variables are defined. Additionally, the Python validation check for modelopt is simplified to use standard Python invocation instead of isolated mode with cleared environment variables.

Changes

Cohort / File(s) Summary
Server Startup Configuration
tools/debugger/server.sh
Moved workdir echo statement to execute earlier in startup (right after CMD_DIR/RESULT_DIR definition). Modified check_modelopt_local validation to invoke Python without isolated mode (-I flag) or explicit PYTHONPATH clearing, while preserving the same validation logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Security Anti-Patterns ❓ Inconclusive Unable to locate relevant security configuration files or PR changes to assess against security anti-patterns. Verify that SECURITY.md exists and that the PR files are accessible for security pattern assessment.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly addresses the main change: fixing the debugger server's failure to detect editable-installed modelopt by removing the problematic PYTHONPATH isolation.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chenjiel/fix_server

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

@cjluo-nv cjluo-nv changed the title Fix debugger server's PYTHONPATH check Fix debugger server failing to detect editable-installed modelopt Apr 16, 2026
Copy link
Copy Markdown
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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tools/debugger/server.sh (1)

102-109: ⚠️ Potential issue | 🟠 Major

check_modelopt_local can false-pass due to PYTHONPATH injection.

PYTHONPATH is exported at line 85 before check_modelopt_local runs (line 112). The python -c invocation at line 102 inherits this environment variable and will import modelopt from $WORKDIR even when editable install is missing, incorrectly skipping pip install -e ".[dev]".

Move the PYTHONPATH export to after the check runs:

Fix
-# Set environment
-export PYTHONPATH="$WORKDIR"
-
 # Check for an already-running server
 if [[ -f "$RELAY_DIR/server.ready" ]]; then
@@
 if check_modelopt_local >/dev/null 2>&1; then
     echo "[server] modelopt already editable-installed from $WORKDIR, skipping pip install."
 else
@@
     echo "[server] Install done."
 fi
+
+# Set environment for executed commands after install/validation
+export PYTHONPATH="$WORKDIR"

Alternatively, use python -I (isolated mode) in the check to prevent PYTHONPATH inheritance.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tools/debugger/server.sh` around lines 102 - 109, The PYTHONPATH export
happening before the local-check allows the python -c check (the inline python
invocation that imports modelopt and compares os.path.realpath against $WORKDIR)
to inherit PYTHONPATH and false-pass; either move the PYTHONPATH export (the
export of PYTHONPATH at line ~85) to after the check so the check runs with the
normal environment, or change the check invocation to isolated mode by using
python -I for the inline script (the python -c "import modelopt..." invocation)
so it ignores PYTHONPATH during import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tools/debugger/server.sh`:
- Around line 102-109: The PYTHONPATH export happening before the local-check
allows the python -c check (the inline python invocation that imports modelopt
and compares os.path.realpath against $WORKDIR) to inherit PYTHONPATH and
false-pass; either move the PYTHONPATH export (the export of PYTHONPATH at line
~85) to after the check so the check runs with the normal environment, or change
the check invocation to isolated mode by using python -I for the inline script
(the python -c "import modelopt..." invocation) so it ignores PYTHONPATH during
import.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e9f2eb7e-97fd-4e45-a65d-803b65262867

📥 Commits

Reviewing files that changed from the base of the PR and between 361f7e3 and 4257ef4.

📒 Files selected for processing (1)
  • tools/debugger/server.sh

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 16, 2026

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-04-16 05:54 UTC

@cjluo-nv cjluo-nv requested review from kaix-nv and meenchen April 16, 2026 00:42
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.66%. Comparing base (361f7e3) to head (4257ef4).
⚠️ Report is 2 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (361f7e3) and HEAD (4257ef4). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (361f7e3) HEAD (4257ef4)
unit 2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1270       +/-   ##
===========================================
- Coverage   75.58%   55.66%   -19.93%     
===========================================
  Files         459      458        -1     
  Lines       48528    48464       -64     
===========================================
- Hits        36681    26976     -9705     
- Misses      11847    21488     +9641     
Flag Coverage Δ
unit 52.02% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cjluo-nv cjluo-nv merged commit d45219b into main Apr 16, 2026
49 of 51 checks passed
@cjluo-nv cjluo-nv deleted the chenjiel/fix_server branch April 16, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants