fix: explicit PYTHONPATH for isolated test subprocesses (#593)#594
Conversation
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
planetf1
changed the title
planetf1
force-pushed
the
fix/subprocess-pythonpath
branch
from
e71015c to
7fadb7d
Compare
docs/examples/conftest.py
Outdated
|
|
||
| repo_root = str(pathlib.Path(__file__).parent.parent.parent.resolve()) | ||
| env = os.environ.copy() | ||
| env["PYTHONPATH"] = f"{repo_root}{os.pathsep}{env.get('PYTHONPATH', '')}" |
There was a problem hiding this comment.
One concern: Will this cause issues if someone has a PYTHONPATH already set? I'm guessing it would only cause issues if someone has some weird setup. To hedge against that, should we set the variable with PYTHONPATH=env.get('PYTHONPATH', "")<; or :>{repo_root}{os.pathsep}.
At the very least, I don't think it makes sense adding PYTHONPATH to the repo root if already exists. I think that will jumble the path.
There was a problem hiding this comment.
I think you can set it to have multiple paths (that's what I was trying to do with my quick example); you set it to the existing path if it exists and then add a link to the base repo as well
|
Thanks @jakelorocco! Ok - I'd originally opted more for isolation (so reverse order) but am ok we go with existing PYTHONPATH configurations as a priority. I've updated to: This is less invasive and only adds repo_root when the local package isn't already accessible. Changes pushed in commit 4b287b3. |
Put existing PYTHONPATH first, then append repo_root as fallback. This is less invasive to user environments while still ensuring the local package is available when needed.
planetf1
force-pushed
the
fix/subprocess-pythonpath
branch
from
4b287b3 to
3748054
Compare
jakelorocco
left a comment
jakelorocco
left a comment
There was a problem hiding this comment.
lgtm assuming tests pass; and apologies if I made you do extra work; I'm realizing now that I may have misread the initial code given your comment
2 participants
Explicit PYTHONPATH for isolated test subprocesses
Type of PR
Description
Fixes #593
Fixes an issue where tests spawned in isolated subprocess environments (such as via
uv run pytest) fail withModuleNotFoundError. The parent pytest process dynamically injects the package path intosys.path, but this memory state is not naturally inherited by childsubprocess.Popencalls. This change explicitly resolves the repository root and injects it into thePYTHONPATHenvironment variable for both the heavy GPU test runner and the examples runner.Verified locally, and confirmed working for both HuggingFace and vLLM test suites on LSF HPC nodes.
Testing