Include more-itertools in build env #8611
Conversation
Signed-off-by: Yun Liu <[email protected]>
WalkthroughAdded Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🔇 Additional comments (1)
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (windows-latest)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds more-itertools>=8.0 as a build-time dependency to fix a ModuleNotFoundError that occurs during package installation. The error happened because setuptools' internal import chain requires more-itertools, but it wasn't available in the build environment.
Changes:
- Added
more-itertools>=8.0to[build-system].requiresinpyproject.toml
Analysis:
The fix correctly addresses issue #8610 by ensuring more-itertools is installed before setup.py runs. The version constraint >=8.0 is reasonable and aligns with modern setuptools requirements. This is a minimal, targeted fix that resolves the build failure without introducing unnecessary dependencies.
Confidence Score: 5/5
- This PR is safe to merge with no risk - it's a minimal build configuration fix
- Score reflects that this is a single-line addition to resolve a well-documented build failure, with no code logic changes, no breaking changes, and the fix directly addresses the root cause identified in the error traceback
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| pyproject.toml | 5/5 | Added more-itertools>=8.0 to build-system requirements to fix setuptools import dependency |
Sequence Diagram
sequenceDiagram
participant Build as Build System (pip/build)
participant PyProject as pyproject.toml
participant Setup as setup.py
participant Setuptools as setuptools
participant Jaraco as jaraco.functools
participant MoreIter as more-itertools
Build->>PyProject: Read [build-system].requires
PyProject-->>Build: wheel, setuptools, more-itertools>=8.0, torch, ninja, packaging
Build->>MoreIter: Install more-itertools>=8.0
Build->>Setuptools: Install setuptools
Build->>Setup: Execute setup.py
Setup->>Setuptools: Import setuptools (line 21)
Setuptools->>Jaraco: Import jaraco.functools
Jaraco->>MoreIter: Import more_itertools
Note over Jaraco,MoreIter: Previously failed here with ModuleNotFoundError
MoreIter-->>Jaraco: Module loaded successfully
Jaraco-->>Setuptools: Import successful
Setuptools-->>Setup: Import successful
Setup->>Build: Package build completes
1 file reviewed, no comments
Signed-off-by: Yun Liu <[email protected]>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR addresses a build failure in the min-dep-pytorch CI job caused by newer versions of setuptools requiring more-itertools as an internal dependency. The fix adds more-itertools>=8.0 installation before running setup.py.
Key Changes:
- Added
more-itertools>=8.0to the pip installation step in themin-dep-pytorchworkflow job - Also updated
pyproject.tomlto includemore-itertools>=8.0in the build-system requirements (previous commit)
Potential Issues:
- The other two jobs in this workflow (
min-dep-osandmin-dep-py3) also callsetup.pyand may encounter the same error min-dep-py3explicitly upgrades setuptools, making it especially vulnerable to this issue- Consider applying the same fix to all three jobs for consistency
Confidence Score: 3/5
- This PR is safe to merge but may not fully resolve the issue across all CI jobs
- The fix correctly addresses the immediate build failure in the
min-dep-pytorchjob by installingmore-itertoolsbefore setuptools attempts to import it. However, the same issue could occur in the other two jobs (min-dep-osandmin-dep-py3) that also runsetup.py. Themin-dep-py3job is particularly at risk since it explicitly upgrades setuptools. .github/workflows/pythonapp-min.yml- Consider applying themore-itertoolsfix to all three workflow jobs for consistency
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| .github/workflows/pythonapp-min.yml | 3/5 | Added more-itertools>=8.0 installation to fix setuptools import error in min-dep-pytorch job. The other two jobs (min-dep-os and min-dep-py3) may need the same fix. |
Sequence Diagram
sequenceDiagram
participant GH as GitHub Actions
participant Env as Python Environment
participant Pip as pip
participant Setup as setuptools
participant Script as setup.py
GH->>Env: Set up Python 3.9
GH->>Pip: pip install --user --upgrade pip setuptools wheel
Note over Pip,Setup: setuptools is upgraded
GH->>Pip: pip install --user more-itertools>=8.0
Note over Pip: Fix: Install more-itertools<br/>before setup.py runs
GH->>Script: python setup.py develop
Script->>Setup: from setuptools import find_packages, setup
Setup->>Setup: Import internal modules
Note over Setup: setuptools requires<br/>more_itertools internally
Setup-->>Script: Import successful
Script-->>GH: MONAI installed in dev mode
Additional Comments (2)
-
.github/workflows/pythonapp-min.yml, line 90 (link)logic:
min-dep-py3job also installs setuptools and callssetup.py(line 109), so it may encounter the samemore_itertoolsimport error -
.github/workflows/pythonapp-min.yml, line 41 (link)style:
min-dep-osjob callssetup.py(line 60) which imports setuptools, potentially encountering the samemore_itertoolserror. Consider addingmore-itertools>=8.0here as well
1 file reviewed, 2 comments
|
/build |
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
I think it's fine to add this dependency and we'll revisit how dependencies are specifed later, eg. setuptools.
2 participants
Fixes #8610.
Description
setuptools’ import chain requires more-itertools, but it isn’t installed when setup.py runs.
Edit pyproject.toml to include it under [build-system].
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.