do not export PYTHONPATH by default in Flux commands
#6597
Conversation
|
Requesting a review from either @trws or @jameshcorbett to vet the changes here just to get someone with more Python experience to give their perspective - however it would be nice to get this fixed in tomorrow's release if this looks good. |
There was a problem hiding this comment.
This seems to me like it requires more environment-variable / process-manipulation knowledge than Python knowledge, but anyway I looked through this and did some research to make sure there isn't another way to manipulate sys.path--I couldn't find one. This looks like a nice improvement to me.
There was a problem hiding this comment.
This all looks right to me and seems like it should solve that problem, though I admit I'm still chewing on two thoughts, both centered around whether we might actually want to set PYTHONPATH even less.
- We have classically set
PYTHONPATHinflux envbecause that's how we set up the environment for any given flux command orflux run. Maybe better to have it be a separate change, or see what kind of fallout it would have, but I'm tempted to say we should remove it from there as well, or perhaps put it behind a--pythonpathflag or similar. - How bad would it be to get the scripts run like flux python commands/scripts? I'm guessing that's a non-trivial amount of work, but setting
PYTHONPATHfor the whole test suite could hide issues where we need to wrap with our python runner or similar and forget.
|
Great points @trws and I was thinking about both of them as well. For 1., I initially did not have
So I figured we should keep it in Similar thinking for all the Python scripts in the testsuite. I agree with your assessment, and perhaps we should open an issue to remove the forced |
|
That all sounds right to me, maybe throw up an issue so we don't lose track of it referencing the docs and test suite? |
|
Yeah, it also occurs to me that |
|
Ok, opened #6600 |
|
I'll set MWP now. |
Problem: src/cmd/builtin/python.c includes "config.h" twice. Remove the extraneous include.
Problem: The py-runner.py script is invoked by flux(1) for any Python subcommand in order to ensure the subcommand has a sane sys.path matching the currently invoked version of Flux. Ideally, this would not require flux(1) to modify PYTHONPATH, which is then inherited by the user's environment (e.g. in the case of `flux run`). However, `py-runner.py` does need the correct PYTHONPATH at startup so it can `import flux`, so PYTHONPATH must be set. In order to avoid polluting the user's environment, save the original callers PYTHONPATH in FLUX_PYTHONPATH_ORIG in flux(1), then restore it if set in `py-runner.py`. If FLUX_PYTHONPATH_ORIG is not set, this means that the callers PYTHONPATH was empty, so unset the variable in this case.
Problem: There are a few builtin subcommands that may make use of the internal function that modifies PYTHONPATH in the environment. Export builtin_env_add_pythonpath() in bultin.h.
Problem: Users that run Python programs using a different version or installation of Python under `flux run` currently have to work around the fact that `flux(1)` always prepends the path to the current Python bindings to `PYTHONPATH`. This gets exported in the job environment and the incorrect Flux Python bindings are loaded, causing errors. The `flux(1)` command ostensibly prepends to `PYTHONPATH` to ensure that Python subcommands use the right version of the bindings, but the `py-runner.py` wrapper now takes care of that, so doing this is actually no longer necessary. Do not modify `PYTHONPATH` in `flux` unless `FLUX_PYTHONPATH_PREPEND` is set. For backwards compatibility, continue to prepend the builtin Python path in the `flux python` and `flux env` utilities. Fixes flux-framework#6594 fixup! cmd: do not always prepend to `PYTHONPATH` in `flux(1)`
Problem: Some Flux manuals describe the `flux(1)` will modify `PYTHONPATH`, but this is no longer true in most situations. Update documentation to reflect reality.
Problem: The landing page for the Flux Python documentation notes that pip can't be used to install the Python bindings, but this is no longer true. Update the note at the top to say there's a `flux-python` pip package available.
Problem: The pyton/basics.rst documentation mentions that `flux` will set `PYTHONPATH` in the environment of all commands, but this is no longer true. Remove the outdated statement.
Problem: Now that `flux(1)` no longer exports PYTHONPATH to everything, some scripts in the teststuie that are not run under `flux python` or as flux subcommands may fail because they load the wrong set of Python modules. Export PYTHONPATH to sharness scripts via sharness.d/01-setup.sh to avoid this issue.
Problem: The testsuite does not ensure that PYTHONPATH is not exported by default by flux(1), but is for the `python` and `env` subcommands. Add tests to t1102-cmddriver.t that verify the expected handling of PYTHONPATH by flux commands.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6597 +/- ##
=======================================
Coverage 79.53% 79.54%
=======================================
Files 531 531
Lines 88170 88213 +43
=======================================
+ Hits 70130 70165 +35
- Misses 18040 18048 +8
|
This PR attempts to fix #6594 while preserving a bit of backwards compatibility and ensuring that Python subcommands get a
sys.paththat will load the correct version of the Flux Python bindings. The steps taken to accomplish this were the following:PYTHONPATHbefore executing thepy-runner.pyPython subcommand wrapper so thatPYTHONPATHcan be modified before running the wrapper. The wrapper script then reestablishes the originalPYTHONPATHaftersys.pathis internally set up correctly. This is necessary so thatpy-runner.pyimports the correct_fluxcffi modules, from which it derives other necessary paths.PYTHONPATHinflux envandflux python(in addition to the case above). Thus,PYTHONPATHis no longer modified in the environment of jobs orflux start.PYTHONPATHno longer has the path to the build tree Python bindings in the testsuite, even underflux start, modifyPYTHONPATHfor sharness tests insharness.d/01-setup.shso that non-Flux subcommand Python scripts run as part of the testsuite use the builddir Python bindings and not system bindings.