Modernising Build Process#9010
Open
ericspod wants to merge 23 commits into
Open
Conversation
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
…requirements 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>
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>
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThe project centralizes package and build metadata in Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
monai/_version.py (1)
47-640: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMake generated definitions conform durably.
These modified definitions lack the required Google-style argument/return/exception documentation. Update the Versioneer template or exempt generated
monai/_version.py; hand edits will be overwritten. As per path instructions, definitions require Google-style variable, return, and exception documentation.🤖 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/_version.py` around lines 47 - 640, The generated Versioneer definitions lack the required Google-style variable, return, and exception documentation. Update the Versioneer template that generates symbols such as get_config, run_command, git_pieces_from_vcs, render, and get_versions, or add the generated monai/_version.py path to the documentation exemption; do not hand-edit the generated file because regeneration will overwrite it.Source: Path instructions
runtests.sh (1)
139-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuote
$REQand clean up the temp file.Unquoted
$REQ(Lines 145-146) is unsafe ifTMPDIRcontains spaces/glob chars, and the temp requirements file is never removed after use.🧹 Proposed fix
REQ=$(mktemp --tmpdir XXX.txt) - ${cmdPrefix}"${PY_EXE}" monai/config/print_dependencies.py all testing > $REQ - ${cmdPrefix}"${PY_EXE}" -m pip install -r $REQ + ${cmdPrefix}"${PY_EXE}" monai/config/print_dependencies.py all testing > "$REQ" + ${cmdPrefix}"${PY_EXE}" -m pip install -r "$REQ" + rm -f "$REQ" }🤖 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 `@runtests.sh` around lines 139 - 146, Update install_deps to quote "$REQ" wherever the temporary requirements file is passed to pip, and remove the temporary file after installation completes. Keep the existing dependency-generation and installation flow unchanged.Source: Linters/SAST tools
docs/source/installation.md (1)
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code block.
Markdownlint flags this block for missing a fence language (MD040).
🤖 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 `@docs/source/installation.md` around lines 54 - 56, Add a language identifier to the fenced code block containing the dependency list in the installation documentation, using an appropriate plain-text language such as text; leave the dependency contents unchanged.Source: Linters/SAST tools
🤖 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 @.github/workflows/cicd_tests.yml:
- Around line 277-282: Update the dependency installation steps in the workflow
to install only `.[all,testing]`, then explicitly install `omegaconf` and
`nvsubquadratic>=0.1.1` with `--no-deps`. Remove the separate `python -m pip
install -e .[hyena]` step and keep the existing isolation strategy.
In @.github/workflows/cron.yml:
- Line 189: Update the artifact-install step in the workflow to install the
built package by its package name variable, using the extras all and testing,
instead of installing the current directory. Remove the python -m pip install
.[all,testing] command and the obsolete requirements*.txt copy from this step.
- Line 242: Update the dependency installation steps in the workflow to complete
the pip-only migration: replace both the existing pip install and BUILD_MONAI
setup.py develop commands with BUILD_MONAI=1 python -m pip install -e
.[all,testing].
In @.github/workflows/integration.yml:
- Line 40: Update the container image used by both integration jobs in
.github/workflows/integration.yml at lines 40-40 and 115-115 to the same image
providing Python 3.10 or newer, before the python -m pip install steps; no other
workflow behavior needs to change.
In `@Dockerfile.slim`:
- Around line 49-56: Update the editable install command after the BUILD_MONAI
and FORCE_CUDA environment settings to include pip’s --no-build-isolation
option, matching the established Dockerfile installation pattern and ensuring
extensions build against the preinstalled runtime dependencies.
In `@docs/source/installation.md`:
- Around line 52-58: Update the corresponding packages list in the installation
documentation by adding cucim-cu12 and cucim-cu13 immediately after clearml and
before cupy, preserving the extras list order so cucim has a 1:1 mapping with
its Pip dependencies.
In `@monai/__init__.py`:
- Around line 65-66: Update the __revision_id__ assignment to preserve an absent
full-revisionid as an empty string instead of converting None to "None", while
leaving the existing version fallback unchanged.
In `@monai/config/print_dependencies.py`:
- Around line 29-57: The parse_dependencies function mutates the caller-owned
sections collection via sections.remove and uses a mutable default. Change
sections to default to None, create a local set from the provided sections, and
perform BUILD_SYSTEM_KEY removal and subsequent membership/iteration operations
on that local copy.
In `@pyproject.toml`:
- Around line 72-73: Update the MetricsReloaded dependency entries for both the
all and metrics_reloaded extras to use the same immutable commit reference
instead of the differing mutable branches. Remove the branch-based URLs and
ensure both entries resolve to that identical pinned commit.
---
Nitpick comments:
In `@docs/source/installation.md`:
- Around line 54-56: Add a language identifier to the fenced code block
containing the dependency list in the installation documentation, using an
appropriate plain-text language such as text; leave the dependency contents
unchanged.
In `@monai/_version.py`:
- Around line 47-640: The generated Versioneer definitions lack the required
Google-style variable, return, and exception documentation. Update the
Versioneer template that generates symbols such as get_config, run_command,
git_pieces_from_vcs, render, and get_versions, or add the generated
monai/_version.py path to the documentation exemption; do not hand-edit the
generated file because regeneration will overwrite it.
In `@runtests.sh`:
- Around line 139-146: Update install_deps to quote "$REQ" wherever the
temporary requirements file is passed to pip, and remove the temporary file
after installation completes. Keep the existing dependency-generation and
installation flow unchanged.
🪄 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: 27887d1b-aeae-4dea-b5b2-726433564b42
📒 Files selected for processing (27)
.github/workflows/build_docs.yml.github/workflows/cicd_tests.yml.github/workflows/codeql-analysis.yml.github/workflows/cron-ngc-bundle.yml.github/workflows/cron.yml.github/workflows/docker.yml.github/workflows/integration.yml.github/workflows/pythonapp-gpu.yml.github/workflows/pythonapp-hyena-gpu.yml.github/workflows/release.yml.github/workflows/weekly-preview.ymlDockerfileDockerfile.slimdocs/requirements.txtdocs/source/installation.mdenvironment-dev.ymlmonai/__init__.pymonai/_version.pymonai/config/print_dependencies.pypyproject.tomlrequirements-dev.txtrequirements-min.txtrequirements.txtruntests.shsetup.cfgsetup.pyversioneer.py
💤 Files with no reviewable changes (6)
- requirements-dev.txt
- setup.cfg
- .github/workflows/docker.yml
- requirements-min.txt
- requirements.txt
- .github/workflows/release.yml
ericspod
commented
Jul 25, 2026
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
for more information, see https://pre-commit.ci
Member
Author
|
@coderabbitai review again. |
Contributor
|
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
pyproject.toml (1)
74-75: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMetricsReloaded still pinned to two different forks/branches.
allusesgit+https://github.com/ericspod/MetricsReloaded@monai-support-fixwhilemetrics_reloadedusesgit+https://github.com/Project-MONAI/MetricsReloaded@monai-support— different repos and branches for the same package. Installing both extras together (or comparing environments built from each) yields divergent, unpinned dependency states. Unify on one immutable commit reference for both extras.Also applies to: 131-131
🤖 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 `@pyproject.toml` around lines 74 - 75, Unify the MetricsReloaded dependency declarations in the all and metrics_reloaded extras to use the same repository and immutable commit reference. Update both entries consistently, replacing the differing fork/branch URLs while preserving the existing package specification structure.
🧹 Nitpick comments (1)
monai/config/print_dependencies.py (1)
1-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for this new dependency-parsing utility.
This script now drives dependency installation across CI, Docker, and
runtests.sh; a parsing regression would break every build path. Consider adding a test that exercisesparse_dependenciesagainst a small sample toml (core, build-system, and optional-dependency sections).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 `@monai/config/print_dependencies.py` around lines 1 - 68, Add unit coverage for parse_dependencies using a temporary minimal pyproject.toml containing project dependencies, build-system.requires, and multiple optional-dependencies. Assert core dependencies are always returned, build-system entries are included only for the build-system section, selected optional sections and "*" behave correctly, and results are deduplicated and sorted.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 @.github/workflows/cicd_tests.yml:
- Around line 143-147: Update the min-dep job’s installation commands to include
the generated minimum constraints from requirements-min.txt, especially for the
all path, while retaining the existing testing extras and build-system
dependency installation. Ensure this matrix continues validating against the
pinned minimum dependency versions rather than an unconstrained pandas release.
In `@Dockerfile.slim`:
- Around line 49-56: The editable install command using `pip install` still
enables build isolation, allowing compiled extensions to use build dependencies
that may not match the installed PyTorch ABI. Update the `BUILD_MONAI=1
FORCE_CUDA=1` installation step to disable build isolation, matching the
safeguard used by `Dockerfile`, while preserving the existing extras and
editable-install options.
In `@docs/source/installation.md`:
- Line 54: Update the fenced code block in the installation documentation to
specify the text language identifier by using a text-labeled fence instead of a
bare fence, satisfying Markdownlint MD040.
In `@monai/config/print_dependencies.py`:
- Around line 29-60: Add a Google-style docstring to parse_dependencies
describing the filename and sections parameters, the sorted dependency-list
return value, and KeyError behavior when the TOML data is malformed or required
keys are missing.
In `@runtests.sh`:
- Around line 144-146: Quote the temporary-file variable REQ in both the
print_dependencies.py output redirection and the pip install command to prevent
word splitting and glob expansion. Keep the existing command behavior unchanged.
---
Duplicate comments:
In `@pyproject.toml`:
- Around line 74-75: Unify the MetricsReloaded dependency declarations in the
all and metrics_reloaded extras to use the same repository and immutable commit
reference. Update both entries consistently, replacing the differing fork/branch
URLs while preserving the existing package specification structure.
---
Nitpick comments:
In `@monai/config/print_dependencies.py`:
- Around line 1-68: Add unit coverage for parse_dependencies using a temporary
minimal pyproject.toml containing project dependencies, build-system.requires,
and multiple optional-dependencies. Assert core dependencies are always
returned, build-system entries are included only for the build-system section,
selected optional sections and "*" behave correctly, and results are
deduplicated and sorted.
🪄 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: 0cbe0862-f439-43e7-a08a-928df0db6d74
📒 Files selected for processing (27)
.github/workflows/build_docs.yml.github/workflows/cicd_tests.yml.github/workflows/codeql-analysis.yml.github/workflows/cron-ngc-bundle.yml.github/workflows/cron.yml.github/workflows/docker.yml.github/workflows/integration.yml.github/workflows/pythonapp-gpu.yml.github/workflows/pythonapp-hyena-gpu.yml.github/workflows/release.yml.github/workflows/weekly-preview.ymlDockerfileDockerfile.slimdocs/requirements.txtdocs/source/installation.mdenvironment-dev.ymlmonai/__init__.pymonai/_version.pymonai/config/print_dependencies.pypyproject.tomlrequirements-dev.txtrequirements-min.txtrequirements.txtruntests.shsetup.cfgsetup.pyversioneer.py
💤 Files with no reviewable changes (6)
- requirements-dev.txt
- setup.cfg
- requirements.txt
- .github/workflows/docker.yml
- requirements-min.txt
- .github/workflows/release.yml
for more information, see https://pre-commit.ci
ericspod
marked this pull request as ready for review
July 25, 2026 23:55
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@runtests.sh`:
- Around line 144-146: Update the dependency installation flow in runtests.sh
around the REQ temporary file to always remove it after pip installation,
including when installation fails; use cleanup that executes before returning or
propagating the installation error.
🪄 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: bdb85aa0-f637-4d2b-8912-a6aae35b34f5
📒 Files selected for processing (6)
.github/workflows/cicd_tests.ymldocs/source/installation.mdenvironment-dev.ymlmonai/config/print_dependencies.pypyproject.tomlruntests.sh
💤 Files with no reviewable changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- environment-dev.yml
- monai/config/print_dependencies.py
- docs/source/installation.md
- .github/workflows/cicd_tests.yml
Comment on lines
+144
to
+146
| REQ=$(mktemp --tmpdir XXX.txt) | ||
| ${cmdPrefix}"${PY_EXE}" monai/config/print_dependencies.py all testing > "$REQ" | ||
| ${cmdPrefix}"${PY_EXE}" -m pip install -r "$REQ" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the temporary requirements file after use.
mktemp creates a persistent file, but this function never deletes it. Clean it up on both successful and failed dependency installation paths to avoid accumulating temporary files across repeated runs.
🔧 Proposed fix
REQ=$(mktemp --tmpdir XXX.txt)
- ${cmdPrefix}"${PY_EXE}" monai/config/print_dependencies.py all testing > "$REQ"
- ${cmdPrefix}"${PY_EXE}" -m pip install -r "$REQ"
+ if ! ${cmdPrefix}"${PY_EXE}" monai/config/print_dependencies.py all testing > "$REQ"; then
+ rm -f "$REQ"
+ return 1
+ fi
+ if ! ${cmdPrefix}"${PY_EXE}" -m pip install -r "$REQ"; then
+ rm -f "$REQ"
+ return 1
+ fi
+ rm -f "$REQ"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| REQ=$(mktemp --tmpdir XXX.txt) | |
| ${cmdPrefix}"${PY_EXE}" monai/config/print_dependencies.py all testing > "$REQ" | |
| ${cmdPrefix}"${PY_EXE}" -m pip install -r "$REQ" | |
| REQ=$(mktemp --tmpdir XXX.txt) | |
| if ! ${cmdPrefix}"${PY_EXE}" monai/config/print_dependencies.py all testing > "$REQ"; then | |
| rm -f "$REQ" | |
| return 1 | |
| fi | |
| if ! ${cmdPrefix}"${PY_EXE}" -m pip install -r "$REQ"; then | |
| rm -f "$REQ" | |
| return 1 | |
| fi | |
| rm -f "$REQ" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 145-145: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 146-146: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@runtests.sh` around lines 144 - 146, Update the dependency installation flow
in runtests.sh around the REQ temporary file to always remove it after pip
installation, including when installation fails; use cleanup that executes
before returning or propagating the installation error.
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
1 participant
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.
Fixes #8980.
Description
This updates the way MONAI is built to be more modern, relying on
pipfor everything with all information consolidated into thepyproject.tomlfile. This is still in progress and requires more testing, and needs to be double checked that the optional dependencies are correct. Highlights:pyproject.toml.setup.cfgfile.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.