feat: better build_wheel.py venv handling #4525
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the virtual environment handling in build_wheel.py by recognizing pre‐existing environments and warning users about potential PyTorch conflicts.
- Introduces a new function create_venv for creating venvs.
- Adds a no-venv option to use the current Python environment as is.
- Updates venv path resolution and incorporates a check/warning for NVIDIA PyTorch issues.
Comments suppressed due to low confidence (1)
scripts/build_wheel.py:76
- [nitpick] The change from using a variable name like venv_dir to venv_prefix could reduce clarity. Consider using a consistent and self-explanatory name (e.g., venv_dir) throughout the code.
venv_prefix = project_dir / f".venv-{py_major}.{py_minor}"
|
/bot run |
|
PR_Github #5980 [ run ] triggered by Bot |
|
PR_Github #5980 [ run ] completed with state |
tongyuantongyu
force-pushed
the
ytong/build_wheel_venv
branch
from
8da545e to
976417b
Compare
|
/bot run |
tongyuantongyu
force-pushed
the
ytong/build_wheel_venv
branch
from
976417b to
46f99cb
Compare
|
PR_Github #5987 [ run ] triggered by Bot |
Signed-off-by: Yuan Tong <13075180+tongyuantongyu@users.noreply.github.com>
tongyuantongyu
force-pushed
the
ytong/build_wheel_venv
branch
from
46f99cb to
b1ad203
Compare
|
/bot run |
|
PR_Github #5989 [ run ] triggered by Bot |
|
PR_Github #5987 [ run ] completed with state |
|
PR_Github #5989 [ run ] completed with state |
| if os.environ.get( | ||
| "NVIDIA_PYTORCH_VERSION") is not None and has_non_system_pytorch: | ||
| warnings.warn( | ||
| "You are using the NVIDIA PyTorch container, but not using the provided PyTorch installation. " |
There was a problem hiding this comment.
Refined by Owen:
Using the NVIDIA PyTorch container without leveraging the included PyTorch installation may lead to compatibility issues.
If you encounter any problems, please delete the environment at `{venv_prefix}` so that `build_wheel.py` can recreate the virtual environment.
| parser.add_argument( | ||
| "--no-venv", | ||
| action="store_true", | ||
| help="Use current Python interpreter as is, don't create venv.") |
There was a problem hiding this comment.
Use the current Python interpreter without creating a virtual environment
| f"If anything goes wrong, please remove the environment at {venv_prefix} " | ||
| "to let build_wheel.py rebuild virtual environment.") | ||
| print("^^^^^^^^^^ IMPORTANT WARNING ^^^^^^^^^^", file=sys.stderr) | ||
| time.sleep(15) |
There was a problem hiding this comment.
Could we use some more elegant behaviour instead of time.sleep(15)?
There was a problem hiding this comment.
Instead of prompting the user to consider taking action, can we just run pip remove torch inside the venv in this case (before running pip install -r "{requirements_file}")?
Some user experience improvements for
build_wheel.pyrelated to virtual environment handling