Draft
Conversation
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds Windows CI support to the build pipeline and fixes Windows-specific test failures.
Major changes:
- Added
test-general-windowsjob in.github/workflows/build.ymlthat runs on self-hosted Windows GPU runners with PowerShell setup - Fixed Windows file locking issues in test files by properly closing
tempfile.NamedTemporaryFilehandles before use and addingPermissionErrorretry logic - Added new
check-links.ymlworkflow for automated documentation link validation using lychee - Updated outdated documentation URLs for Isaac Sim forums and ETH Zurich Euler cluster
The Windows CI job installs Python 3.11, Isaac Sim 5.1.0, PyTorch with CUDA 12.8, and runs general tests excluding isaaclab_tasks. Results are uploaded as artifacts and combined with Linux test results.
Confidence Score: 4/5
- Safe to merge with one syntax issue to verify in the Windows CI job
- The PR makes solid improvements for Windows compatibility with proper file handle management and adds comprehensive CI infrastructure. Test file fixes follow correct Windows patterns (close handles, retry on PermissionError). However, there's a potential syntax issue with PowerShell backtick escaping in the Python multiline command that should be verified - Python's
-cflag expects actual newlines, not\n` sequences. .github/workflows/build.ymlline 200 - verify the PowerShell Python command works correctly with backtick-escaped newlines
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| .github/workflows/build.yml | 4/5 | Added Windows CI job with PowerShell setup, test execution, and artifact handling; integrated into existing workflow |
| scripts/tools/test/test_cosmos_prompt_gen.py | 5/5 | Fixed Windows-specific file handle issues by closing temp files before use and adding PermissionError retry logic |
| scripts/tools/test/test_hdf5_to_mp4.py | 5/5 | Fixed Windows file locking by closing temp file handles before h5py access and adding PermissionError retry logic |
| scripts/tools/test/test_mp4_to_hdf5.py | 5/5 | Fixed Windows file locking by closing temp files immediately and explicitly closing h5py file objects |
Sequence Diagram
sequenceDiagram
participant PR as Pull Request
participant GHA as GitHub Actions
participant Linux as Linux Runner
participant Windows as Windows Runner
participant Combine as Combine Results
PR->>GHA: Trigger on PR
par Parallel Test Execution
GHA->>Linux: test-isaaclab-tasks job
Linux->>Linux: Build Docker image
Linux->>Linux: Run IsaacLab tasks tests
Linux->>GHA: Upload test results artifact
GHA->>Linux: test-general job
Linux->>Linux: Build Docker image
Linux->>Linux: Run general tests (exclude isaaclab_tasks)
Linux->>GHA: Upload test results artifact
GHA->>Windows: test-general-windows job
Windows->>Windows: Checkout code
Windows->>Windows: Create/verify Python 3.11 venv
Windows->>Windows: Install Isaac Sim 5.1.0
Windows->>Windows: Install PyTorch + CUDA 12.8
Windows->>Windows: Install Isaac Lab dependencies
Windows->>Windows: Run general tests via isaaclab.bat
Windows->>Windows: Generate test report XML
Windows->>GHA: Upload Windows test results artifact
end
GHA->>Combine: combine-results job
Combine->>GHA: Download all test artifacts
Combine->>Combine: Merge XML reports
Combine->>GHA: Upload combined results
Combine->>PR: Post test results comment
8 files reviewed, 1 comment
|
|
||
| # Check if Isaac Sim is installed | ||
| Write-Host "Checking if Isaac Sim is already installed..." | ||
| $isaacsimCheck = python -c "try:`n import isaacsim`n print('installed')`nexcept:`n print('not_installed')" 2>&1 |
Contributor
There was a problem hiding this comment.
syntax: PowerShell backtick escaping in Python multiline string may not work correctly. Python's -c expects actual newlines, not backtick-n sequences.
Suggested change
| $isaacsimCheck = python -c "try:`n import isaacsim`n print('installed')`nexcept:`n print('not_installed')" 2>&1 | |
| $isaacsimCheck = python -c @" | |
| try: | |
| import isaacsim | |
| print('installed') | |
| except: | |
| print('not_installed') | |
| "@ 2>&1 |
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Signed-off-by: Kelly Guo <kellyguo123@hotmail.com>
2ef7fc8 to
f3061a4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there