-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(isaaclab.bat) avoid CMD window-title quote rule breaking for /f subprocess #4016
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
base: main
Are you sure you want to change the base?
Conversation
Greptile OverviewGreptile SummaryFixed CMD quote-handling bug in Windows batch script that prevented PyTorch version detection from working correctly.
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant isaaclab.bat
participant ensure_cuda_torch
participant CMD
participant pip
User->>isaaclab.bat: Run with -i flag
isaaclab.bat->>ensure_cuda_torch: Call :ensure_cuda_torch
ensure_cuda_torch->>pip: Check if torch installed
alt torch already installed
ensure_cuda_torch->>CMD: Execute for /f loop to get version
Note over CMD: Before fix: quotes around !python_exe!<br/>caused CMD to strip outer quotes<br/>breaking the command
CMD->>pip: Run pip show torch | findstr
pip-->>CMD: Return version string
CMD-->>ensure_cuda_torch: Set TORCH_CUR variable
ensure_cuda_torch->>ensure_cuda_torch: Compare versions
alt version mismatch
ensure_cuda_torch->>pip: Uninstall old torch
ensure_cuda_torch->>pip: Install correct torch version
end
else torch not installed
ensure_cuda_torch->>pip: Install torch with CUDA
end
ensure_cuda_torch-->>isaaclab.bat: Return success
isaaclab.bat->>User: Continue installation
|
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.
1 file reviewed, no comments
|
Apologies, I just realized that the original use of double quotes "" might have been to avoid errors caused by spaces in the path. Based on my tests, after modifying my PR, it does indeed cause errors when there are spaces in the ISAACLAB_PATH. However, other for /f statements in the isaaclab.bat script seem to use the version without quotes, so the issue might actually be in line with the “consistent behavior” across the script? I suspect a better solution might be to clarify in the documentation not to place IsaacLab in a path with spaces (or make more significant changes to ensure there are no errors in such cases). |
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.
1 file reviewed, no comments
Description
This PR fixes an issue in the Windows batch script responsible for detecting the
torchversion.Inside
:ensure_cuda_torch, thefor /f ('...')construct wraps the inner command with an additional layer, resulting in something like:This triggers the quote-handling behavior of cmd /c, causing the outer pair of quotes to be stripped. As a result, the actual inner command becomes something like:
This leads to the error:
The filename, directory name, or volume label syntax is incorrect.Considering that this fix is extremely small and straightforward, I did not open a separate issue for it (please let me know if this aligns with the contribution guidelines). Also, because the change is minimal and the bug itself is not severe, I did not add my name to CONTRIBUTOR.md.
Type of change
Screenshots
Before the fix (my Windows system is in Chinese, so the error message is localized):


After the fix:
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there