[None][feat] Add --skip_requirements to build_wheel.py#16188
[None][feat] Add --skip_requirements to build_wheel.py#16188lucifer1004 wants to merge 1 commit into
Conversation
… managed environments build_wheel.py unconditionally pip-installs requirements-dev.txt (and conan) into the build environment. When that environment is managed by an external tool (conda, pixi, uv) whose resolution deliberately differs from the requirement pins — for example a torch or NCCL version override — every build silently downgrades or replaces the managed packages, and the external manager restores them on its next sync, masking what happened. With --skip_requirements the environment's installed packages are used as they are: the dev-requirements install and the conan install are both skipped, while conan is still located (venv scripts directory, then PATH) and its absence still fails loudly. Typically combined with --no-venv. Default behavior is unchanged. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Zihua Wu <13583761+lucifer1004@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds a ChangesSkip Requirements Installation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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 `@scripts/build_wheel.py`:
- Around line 138-150: Add a guard in setup_venv so skip_requirements cannot be
used with a freshly created virtual environment: when skip_requirements is true,
verify no_venv is also true or that the current interpreter is already inside a
venv (sys.prefix != sys.base_prefix), and otherwise fail early with a clear
error. Update the setup_venv flow before create_venv and the later package
checks so the invalid combination is caught near the option handling instead of
letting tensorrt/build installation steps fail later.
🪄 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: Enterprise
Run ID: 82dfc434-c9f2-4da4-b141-9a67447bac5a
📒 Files selected for processing (1)
scripts/build_wheel.py
| def setup_venv(project_dir: Path, | ||
| requirements_file: Path, | ||
| no_venv: bool, | ||
| yes: bool = False) -> tuple[Path, Path]: | ||
| yes: bool = False, | ||
| skip_requirements: bool = False) -> tuple[Path, Path]: | ||
| """Creates/updates a venv and installs requirements. | ||
|
|
||
| Args: | ||
| project_dir: The root directory of the project. | ||
| requirements_file: Path to the requirements file. | ||
| no_venv: Use current Python environment as is. | ||
| skip_requirements: Do not pip-install requirements (or conan) into | ||
| the environment; use its installed packages as they are. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing guard when skip_requirements is combined with a freshly-created venv.
setup_venv only skips pip-install of requirements/conan; it does not check whether no_venv is set. If a user passes --skip_requirements without --no-venv (and is not already inside a venv), create_venv() at Line 160 creates a brand-new, empty venv, and then no packages (torch, build, tensorrt, etc.) are ever installed into it. This is silently allowed even though the help text says it's "typically combined with --no-venv" — the resulting failure (e.g. the tensorrt check a few lines later, or python -m build) will be confusing and won't point back to this flag combination.
Consider raising a clear error/warning in setup_venv when skip_requirements is True but the environment is not the current interpreter (i.e., no_venv is False and sys.prefix == sys.base_prefix), since that combination guarantees a downstream failure.
🛡️ Proposed guard
if no_venv or sys.prefix != sys.base_prefix:
reason = "Explicitly requested by user" if no_venv else "Already inside virtual environment"
print(f"-- {reason}, using environment {sys.prefix} as is.")
venv_prefix = Path(sys.prefix)
else:
+ if skip_requirements:
+ raise RuntimeError(
+ "--skip_requirements requires --no-venv (or running inside an "
+ "existing environment); a freshly created venv would have no "
+ "packages installed.")
venv_prefix = create_venv(project_dir)Also applies to: 215-228
🤖 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 `@scripts/build_wheel.py` around lines 138 - 150, Add a guard in setup_venv so
skip_requirements cannot be used with a freshly created virtual environment:
when skip_requirements is true, verify no_venv is also true or that the current
interpreter is already inside a venv (sys.prefix != sys.base_prefix), and
otherwise fail early with a clear error. Update the setup_venv flow before
create_venv and the later package checks so the invalid combination is caught
near the option handling instead of letting tensorrt/build installation steps
fail later.
1 participant
Currently,
build_wheel.pyunconditionally pip-installsrequirements-dev.txt(and conan) into the build environment. When that environment is managed by an external tool (conda, pixi, uv) whose resolution deliberately differs from the requirement pins — for example a torch or NCCL version override — every build silently downgrades or replaces the managed packages, and the external manager restores them on its next sync, masking what happened.With
--skip_requirementsthe environment's installed packages are used as they are: the dev-requirements install and the conan install are both skipped, while conan is still located (venv scripts directory, then PATH) and its absence still fails loudly. Typically combined with--no-venv. Default behavior is unchanged.Summary by CodeRabbit
New Features
Bug Fixes