fix: allow virtualenv packages to override system markers - #5204
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates merge_virtual_env_packages to allow user-specified packages to override system markers (e.g., #system_torch#) by extracting the underlying package name. The review feedback correctly identifies that if a system marker contains an environment marker (e.g., "; sys_platform == 'linux'"), the suffix check will fail. It is recommended to split the package string by ; before checking prefixes/suffixes, and to expand the test suite to cover this scenario.
ba206db to
5325e95
Compare
|
Follow-up for the The implementation on this branch already splits the requirement at "#system_torchaudio# ; sys_platform == 'linux'"New head: Validation on VENV=/cpfs_speech/user/zhifu.gzf/.cache/funasr-ops/xinference-5204-venv
$VENV/bin/python -m pytest --noconftest \
xinference/core/tests/test_utils.py::test_merge_virtual_env_packages_user_package_overrides_system_marker \
xinference/core/tests/test_utils.py::test_merge_virtual_env_packages_user_package_overrides_marked_system_marker -q
# 2 passed, 1 warning in 3.14s
$VENV/bin/python -m py_compile \
xinference/core/utils.py \
xinference/core/tests/test_utils.py \
xinference/core/tests/test_worker.py
git diff --check |
|
Follow-up for the Windows 3.10 CI failure on the previous head Root cause: Fix pushed in Validation on VENV=/cpfs_speech/user/zhifu.gzf/.cache/funasr-ops/xinference-5204-venv
$VENV/bin/python -m pytest --noconftest \
xinference/deploy/docker/pypiserver/tests/test_scripts.py::test_transformers_optional_dependencies_are_scoped_and_mirrored \
xinference/core/tests/test_utils.py::test_merge_virtual_env_packages_user_package_overrides_system_marker \
xinference/core/tests/test_utils.py::test_merge_virtual_env_packages_user_package_overrides_marked_system_marker -q
# 3 passed, 1 warning in 3.25s
$VENV/bin/python -m py_compile \
xinference/deploy/docker/pypiserver/generate_package_lists.py \
xinference/deploy/docker/pypiserver/tests/test_scripts.py \
xinference/core/utils.py \
xinference/core/tests/test_utils.py
git diff --check |
…-user-overrides-5187 # Conflicts: # xinference/deploy/docker/pypiserver/generate_package_lists.py
|
Lint failed, could you fix it? |
|
Pushed
Failure from the job log:
This test is outside the virtualenv marker override diff and failed only on Windows 3.10 after 791 tests had passed. I added the narrowest CI-stability follow-up: skip this specific concurrent OPT transformers test on Local validation on the follow-up:
The local base environment here lacks |
|
Pushed The failed test is a cluster-backed request-limit REST test and the failure happens before the test body can exercise this PR's system-prompt changes. I scoped the follow-up to skip only Local validation before push: New head is |
|
Rechecked the current head Current status:
I attempted the focused pytest path as well, but this checkout’s |
|
I removed the two unrelated Windows test skips from the final diff. Those skips were reactions to environment-local failures in OPT generation and actor-pool socket binding; they are not part of the virtualenv package-marker fix and should remain upstream test coverage. Current head
Fresh focused validation:
The reviewer-requested behavior remains intact: conditional base markers are preserved until engine filtering, while an explicit user package replaces all matching system-marker variants. |
# Conflicts: # xinference/core/tests/test_utils.py
What this PR changes
This fixes the virtualenv package merge key for
#system_<package>#markers so an explicit user package can replace the corresponding system marker.For example, a launch override such as
virtual_env_packages=["torchaudio==2.13.0"]now replaces#system_torchaudio#instead of being appended after it. That lets users recover audio model launches such as SenseVoiceSmall on hosts wheretorchaudiois not installed in the parent environment.Why
merge_virtual_env_packages()documents that packages with the same name are replaced by user-supplied versions. Before this change,#system_torchaudio#used the whole marker as its merge key whiletorchaudio==...usedtorchaudio, so they did not collide and xoscar still failed while resolving the marker.Fixes #5187.
Tests
Run locally in a minimal venv with
xoscar==0.9.6:I also verified direct
merge_virtual_env_packages()assertions for the existing transformers/numpy override case and the FunASR/SenseVoice torchaudio override case.