fix(scripts): use PYTHON_HISTORY instead of unsupported PYTHONHISTFILE#1774
Conversation
PYTHONHISTFILE is not recognized by CPython. Python 3.13+ uses PYTHON_HISTORY to control the .python_history file location. The sandbox was setting the wrong variable name, so the Python REPL ignored it and wrote history to the default location. Closes NVIDIA#1735 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced the incorrect Python history environment variable name Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Line 68: Update the tests to match the renamed environment variable: replace
assertions that expect PYTHONHISTFILE with PYTHON_HISTORY in
test/service-env.test.ts (specifically the assertions around the snippets you
noted). Search for all occurrences of "PYTHONHISTFILE" in
test/service-env.test.ts (including the two assertion blocks around lines
~170-182 and ~346-358) and change them to expect "PYTHON_HISTORY" so the tests
align with the script change in scripts/nemoclaw-start.sh.
- Line 68: Replace the incorrect environment variable name 'PYTHON_HISTORY' with
the standard Python variable 'PYTHONHISTFILE' in the script (update the string
"'PYTHON_HISTORY=/tmp/.python_history'" to use 'PYTHONHISTFILE'), so Python 3.4+
will pick up the history file; ensure the rest of the line's formatting
(quotes/export) remains consistent with surrounding environment variable
assignments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07891a12-82d4-4770-8fc9-61e8407c07bd
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
|
We need to upgrade the Python version in the sandbox as well; otherwise, the |
|
✨ Possibly related open issues: Possibly related open issues: |
|
@cr7258 Good catch — the sandbox base image pins Python 3.11.2 (Debian bookworm), and This rename is the correct forward-compatible half: once the base image upgrades to 3.13+, the var will take effect without any further script changes. The Python upgrade itself is a bigger lift — bookworm doesn't ship 3.13, so it'd require a base-image or repo change, re-pinning all apt packages, and validating against C API / removed-module breakage across 3.12–3.13. Will leave the Python version bump to maintainer team as it would require more discussion. Cheers! |
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — correct fix. PYTHON_HISTORY is the real CPython 3.13+ env var; PYTHONHISTFILE was never recognized. Tests updated to match.
Summary
Replace
PYTHONHISTFILEwithPYTHON_HISTORYin the sandbox environment variable redirects.Problem
nemoclaw-start.shsetsPYTHONHISTFILE=/tmp/.python_historyto redirect the Python REPL history file to/tmp/. However,PYTHONHISTFILEis not a recognized CPython environment variable. Python 3.13+ usesPYTHON_HISTORYfor this purpose, and earlier versions have no environment variable control at all. The current setting is silently ignored.Fix
One-line change:
PYTHONHISTFILEtoPYTHON_HISTORYon line 68 ofscripts/nemoclaw-start.sh.Test plan
Closes #1735
Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit