Check Environment Script - #9038
Conversation
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: Eric Kerfoot <17726042+ericspod@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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds a standalone environment diagnostic script with platform, package, PyTorch, CUDA, and MONAI checks. Configuration output now uses anonymized, flushed printing, and the deprecated Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
monai/config/check_env.py (1)
57-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the new utility docstrings.
monai/config/check_env.py#L57-L150: Document*argsand**kwargsforfprint. AddReturnssections for Boolean diagnostic checks.monai/config/deviceconfig.py#L54-L61: Document*args,**kwargs, and theNonereturn value forfprint.As per path instructions, “Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/config/check_env.py` around lines 57 - 150, Complete the Google-style docstrings for fprint, print_environment, check_torch, check_torch_cuda, and check_monai in monai/config/check_env.py: document fprint’s *args and **kwargs, and add Returns sections describing the Boolean diagnostic results for each check function. Also update fprint in monai/config/deviceconfig.py to document *args, **kwargs, and its None return value; make no other behavioral changes.Source: Path instructions
tests/config/test_print_info.py (1)
22-25: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest the anonymization contract.
out.tell()only confirms that output exists. Callfprintwith patchedUSERandHOST, then assert that raw values are absent and placeholders are present.As per path instructions, “Ensure new or modified definitions will be covered by existing or new unit tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/config/test_print_info.py` around lines 22 - 25, Update test_print_info to patch USER and HOST with known raw values before calling print_debug_info, then assert the output excludes those values and includes the anonymization placeholders. Replace the output-length-only assertion while preserving coverage of the print_debug_info path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/config/check_env.py`:
- Around line 163-167: Update the environment-reporting conditional around
print_environment_vars() and print_environment() to enter when either args.env
or args.envvars is set, while keeping variable output gated by args.envvars. Add
a CLI regression test covering --envvars without --env and ensure the modified
behavior is covered.
- Around line 102-150: Update check_torch and check_monai to catch runtime
exceptions from their diagnostic operations, report them with efprint, and
return False while preserving the existing ImportError handling. Move the full
check_torch_cuda workflow, including torch.cuda.device_count() and related
reporting, inside exception handling so pre-device CUDA failures are also
reported via efprint and return False.
---
Nitpick comments:
In `@monai/config/check_env.py`:
- Around line 57-150: Complete the Google-style docstrings for fprint,
print_environment, check_torch, check_torch_cuda, and check_monai in
monai/config/check_env.py: document fprint’s *args and **kwargs, and add Returns
sections describing the Boolean diagnostic results for each check function. Also
update fprint in monai/config/deviceconfig.py to document *args, **kwargs, and
its None return value; make no other behavioral changes.
In `@tests/config/test_print_info.py`:
- Around line 22-25: Update test_print_info to patch USER and HOST with known
raw values before calling print_debug_info, then assert the output excludes
those values and includes the anonymization placeholders. Replace the
output-length-only assertion while preserving coverage of the print_debug_info
path.
🪄 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 Plus
Run ID: 4ac837b2-74f0-41bc-9115-8d129c94eb8d
📒 Files selected for processing (7)
.github/workflows/cicd_tests.ymlMANIFEST.inmonai/config/__init__.pymonai/config/__main__.pymonai/config/check_env.pymonai/config/deviceconfig.pytests/config/test_print_info.py
💤 Files with no reviewable changes (1)
- monai/config/init.py
| Print all environment variables other than a few known pointless ones. | ||
| """ | ||
| fprint("Environment:") | ||
| for k, v in os.environ.items(): |
There was a problem hiding this comment.
This prints every environment variable except LS_COLORS/PS1/PS2. Since the module docstring recommends "python check_env.py --env --envvars --monai" as the full-diagnostics command and the script is meant to be run and shared for remote troubleshooting (including the curl-pipe-to-python usage), this risks a user pasting secrets (API keys, tokens, credentials) that happen to be set in their shell into a shared diagnostic dump. Consider redacting values for keys matching common secret patterns (TOKEN, KEY, SECRET, PASSWORD, AUTH, etc.) before printing, or adding a warning when --envvars is used.
| """ | ||
| import torch | ||
|
|
||
| dcount = torch.cuda.device_count() |
There was a problem hiding this comment.
dcount = torch.cuda.device_count() is called before the try block starts. If this call itself raises (e.g. a broken or mismatched CUDA driver -- exactly the kind of environment problem this script exists to diagnose), the script will crash with a raw traceback instead of producing the intended graceful "PyTorch encountered CUDA error" message. Consider moving the device_count() call inside the try block below it.
garciadias
left a comment
garciadias
left a comment
There was a problem hiding this comment.
The PR checklist claims "New tests added to cover the changes," but the new check_env.py script (argument parsing, check_torch, check_torch_cuda, print_environment, print_environment_vars) has no test coverage at all. The only test change updates the pre-existing print_debug_info test to capture output via StringIO. Since this script is also now invoked directly from CI (cicd_tests.yml), consider adding tests/config/test_check_env.py covering at least the CLI flags and the anonymization behavior of fprint.
garciadias
left a comment
garciadias
left a comment
There was a problem hiding this comment.
Two Major findings worth addressing before merge: a potential secret-leak via --envvars, and no test coverage for the new check_env.py script despite the checklist claim. Details in inline/PR comments. Everything else looks solid.
2 participants
Fixes # .
Description
This adds a script to check the environment for installed libraries, if PyTorch works, if MONAI works, and print platform information. This script is meant to be useable without anything being installed other than Python itself, so it can be used to inspect an environment before attempting to install MONAI or produce diagnostic information when some environmental failure occurs. The script can be run remotely with
curlas it only relies on the standard library. Minor tweaks are made to the CI action to use this script rather than the copy-pasted short Python lines.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.