ENH: Specify Python 3.11 for nightly health / remote runner#83
Merged
Conversation
Also, small tweak to data/readme.md
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR updates workflow and ignore settings, rewrites dataset documentation, changes USD type fallback handling, and refreshes dataset references in supporting docs. ChangesRepository docs and runtime updates
Estimated code review effort: 2 (Simple) | ~15 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the nightly “health” GitHub Actions workflow to explicitly create its virtual environment using Python 3.11 (needed for the physicsnemo extra on the Windows GPU runner), and refreshes the top-level data/README.md content describing available datasets and how to obtain them.
Changes:
- Pin venv creation in
nightly-health.ymlto Python 3.11 via the Windowspylauncher. - Add editor/IDE directories (
.cursor,.vscode) to.gitignore. - Rewrite/expand
data/README.mddataset overview and download instructions.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
data/README.md |
Expanded dataset documentation and download guidance. |
.gitignore |
Ignores additional local editor directories. |
.github/workflows/nightly-health.yml |
Creates the workflow venv explicitly with Python 3.11. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/nightly-health.yml (1)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent venv creation approach between workflows.
The nightly-health workflow uses
py -3.11 -m venvwhiletest-slow.yml(lines 23-29) uses& "C:\Program Files\Python311\python.exe" -m venv. Both work on Windows runners with Python 3.11 installed, but the inconsistency could cause confusion. Consider aligning both workflows to the same approach for maintainability.🤖 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 @.github/workflows/nightly-health.yml around lines 57 - 62, The venv setup is inconsistent between the workflow files, so align the nightly-health job’s venv creation in the create-venv step with the same Windows Python 3.11 invocation used by the other workflow, or update both to a single standard approach. Keep the existing Python 3.11 requirement in the nightly-health workflow and make the command style match the corresponding venv setup used in test-slow.yml for easier maintenance.
🤖 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 @.gitignore:
- Around line 16-17: The .vscode ignore rule is too broad and prevents the
documented settings file from being tracked. Update the ignore pattern in
.gitignore so the repository can still commit .vscode/settings.json while
continuing to ignore other local VS Code files; adjust the .gitignore entry for
.vscode accordingly without affecting the .cursor rule.
In `@data/README.md`:
- Around line 7-13: The fenced blocks in this README are missing language tags,
which triggers markdownlint MD040. Update the affected fences in the data
listing sections to include an appropriate language such as text so the blocks
remain lint-clean; apply the same fix to all matching fenced examples in this
file, not just the one shown here.
- Around line 177-199: The KCL dataset setup instructions are incomplete because
the README only mentions average_mesh.vtk and one example mesh, while
DownloadKCLHeartModelData requires the full input_meshes set. Update the dataset
layout section to explicitly list input_meshes/01.vtk through
input_meshes/20.vtk alongside average_mesh.vtk, and make sure the manual
download steps and required files match what DownloadKCLHeartModelData expects.
- Around line 17-29: Update the download-method summary in the README to match
the behavior in download_data.py and DataDownloadTools: Slicer-Heart-CT is not
the only auto-downloadable dataset, since KCL-Heart-Model and CHOP-Valve4D are
also routed through the download helpers. Revise the affected summary section
and the matching table so the dataset download status accurately reflects which
entries are automatically downloaded versus manually provided/preprocessed,
keeping the wording consistent with the download_data.py logic.
---
Nitpick comments:
In @.github/workflows/nightly-health.yml:
- Around line 57-62: The venv setup is inconsistent between the workflow files,
so align the nightly-health job’s venv creation in the create-venv step with the
same Windows Python 3.11 invocation used by the other workflow, or update both
to a single standard approach. Keep the existing Python 3.11 requirement in the
nightly-health workflow and make the command style match the corresponding venv
setup used in test-slow.yml for easier maintenance.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1dc8460-02cb-4e3b-9f1d-05fe738fb1c8
📒 Files selected for processing (3)
.github/workflows/nightly-health.yml.gitignoredata/README.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/physiotwin4d/vtk_to_usd/usd_utils.py (1)
166-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider routing
USHORTthrough the unsigned path for semantic consistency.
DataType.USHORT(unsigned 16-bit) is now converted to signedint32/Vt.IntArray, whileDataType.UINT/DataType.ULONG(unsigned 32/64-bit) are converted touint32/Vt.UIntArray. Althoughint32can hold alluint16values without data loss, this inconsistency means unsigned short primvars will have a signed SDF type (IntArrayvsUIntArray), which could confuse downstream consumers expecting unsigned semantics.If
USHORTis rare in practice, this is low priority, but aligning it with theUINT/ULONGbranch (or at minimum documenting the choice) would improve clarity.🤖 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 `@src/physiotwin4d/vtk_to_usd/usd_utils.py` around lines 166 - 169, The `usd_utils.py` type-mapping branch currently routes `DataType.USHORT` through the signed `int32`/`Vt.IntArray` path, which is inconsistent with the unsigned handling used for `DataType.UINT` and `DataType.ULONG`. Update the conversion logic in the array-to-USD helper so `USHORT` follows the unsigned semantic path (or explicitly document why it must remain signed), keeping the mapping behavior aligned with the surrounding `DataType` branches and `Vt.*Array` choices.
🤖 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.
Nitpick comments:
In `@src/physiotwin4d/vtk_to_usd/usd_utils.py`:
- Around line 166-169: The `usd_utils.py` type-mapping branch currently routes
`DataType.USHORT` through the signed `int32`/`Vt.IntArray` path, which is
inconsistent with the unsigned handling used for `DataType.UINT` and
`DataType.ULONG`. Update the conversion logic in the array-to-USD helper so
`USHORT` follows the unsigned semantic path (or explicitly document why it must
remain signed), keeping the mapping behavior aligned with the surrounding
`DataType` branches and `Vt.*Array` choices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e3d379d-fcb4-4aec-93b3-786f9ecf53cb
📒 Files selected for processing (6)
.gitignoredata/DirLab-4DCT/README.mddata/README.mdsrc/physiotwin4d/vtk_to_usd/usd_utils.pytests/README.mdtutorials/tutorial_01_heart_gated_ct_to_usd.py
✅ Files skipped from review due to trivial changes (4)
- data/DirLab-4DCT/README.md
- .gitignore
- tutorials/tutorial_01_heart_gated_ct_to_usd.py
- tests/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- data/README.md
Comment on lines
+1
to
+3
| # PhysioTwin4D Data Directory | ||
|
|
||
| This directory holds the example datasets used by PhysioTwin4D's tutorials, | ||
| experiments, and tests. Each subdirectory is one dataset and has its own | ||
| `README.md` with the download URL(s), directory layout, data sizes, and the | ||
| exact `physiotwin4d-download-data` command for that dataset. This file is | ||
| just the map — see the subdirectory README before downloading anything. | ||
| This directory contains sample datasets used for experiments, testing, and development of the PhysioTwin4D library. Each subdirectory contains a specific medical imaging dataset. |
Comment on lines
+38
to
+42
| ## Slicer-Heart-CT AUTO-DOWNLOAD | ||
|
|
||
| ### Description | ||
|
|
||
| 4D cardiac CT dataset with temporal gating showing complete cardiac cycle motion. Pediatric cardiac CT with truncal valve visualization. |
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.
Also, small tweak to data/readme.md
Summary by CodeRabbit
Bug Fixes
Documentation
Chores