Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrapper script: consider clearing PYTHONPATH #5124

Closed
oliver-sanders opened this issue Sep 8, 2022 · 9 comments · Fixed by #5727
Closed

wrapper script: consider clearing PYTHONPATH #5124

oliver-sanders opened this issue Sep 8, 2022 · 9 comments · Fixed by #5727
Assignees
Labels
could be better Not exactly a bug, but not ideal. investigation question Flag this as a question for the next Cylc project meeting.
Milestone

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 8, 2022

We advise that Cylc is installed into an isolated environment (e.g. conda / virtualenv). Unfortunately the PYTHONPATH environment variable can corrupt these.

Illustrative example (less extreme cases can cause problems too):

export PYTHONPATH='/path/to/python3.6/site-packages'
cylc version
Traceback ... not quite so isolated after all

This is very inconvenient for users who rely on PYTHONPATH (e.g. to load their own Python environment) as they have to unload this environment before they can run Cylc commands. If Cylc is installed in an isolated environment, then it should be isolated from stuff like this.

We might be able to do something like this:

PYTHONPATH= exec "${CYLC_HOME}/bin/${0##*/}" "$@"

Otherwise, there is the nuclear [Python] option:

-E     : ignore PYTHON* environment variables (such as PYTHONPATH)

Probably a good idea? But it does have the unwelcome side-effect of disabling PYTHONBREAKPOINT.

And also:

-s     : don't add user site directory to sys.path; also PYTHONNOUSERSITE

Could do with a bit of investigation to work out what the consequences of these approaches are.

Pull requests welcome!

@oliver-sanders oliver-sanders added the could be better Not exactly a bug, but not ideal. label Sep 8, 2022
@oliver-sanders oliver-sanders added this to the cylc-8.1.0 milestone Sep 8, 2022
@oliver-sanders
Copy link
Member Author

(Tagged 8.1.0 but could go into 8.0.x if we can agree a solution in time)

@oliver-sanders
Copy link
Member Author

Note: Users might need PYTHONPATH to load modules for rose_ana which could make a mess of things.

@hjoliver
Copy link
Member

hjoliver commented Sep 15, 2022

Unfortunately not everyone uses the wrapper! E.g. single user installations.

What about explicitly cleaning sys.path at start-up? I think that would work in conda environments. In a Python venv though, the system Python libs (used to create the venv) are needed in sys.path.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Sep 15, 2022

I'm not sure that would work? The Python CLI options are probably a better bet.

Note that all Cylc users use a wrapper script, not necessarily the Cylc wrapper script, but the one generated for them by pypi or Conda so we might be able to change things there too.

Unfortunately I think we might actually need to bodge PYTHONPATH for rose_ana which makes a right mess of things.

@oliver-sanders oliver-sanders modified the milestones: cylc-8.1.0, cylc-8.2.0 Nov 15, 2022
@oliver-sanders oliver-sanders modified the milestones: cylc-8.2.0, cylc-8.x Jun 29, 2023
@oliver-sanders oliver-sanders added the question Flag this as a question for the next Cylc project meeting. label Aug 15, 2023
@oliver-sanders oliver-sanders modified the milestones: cylc-8.x, cylc-8.3.0 Aug 15, 2023
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Aug 15, 2023

Note, this impacts Rose worse than Cylc, however, with Rose we actually need the PYTHONPATH to be preserved because it will be required for the Rose "command".

@dpmatthews
Copy link
Contributor

Note, this impacts Rose worse than Cylc, however, with Rose we actually need the PYTHONPATH to be preserved because it will be required for the Rose "command".

Perhaps we need to save the PYTHONPATH in another environment variable which Rose can use before running the "command".

@oliver-sanders
Copy link
Member Author

That's sounding like the best option, note the variable would have to be set in the wrapper-script and restored in the rose library code.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Sep 4, 2023

Suggestion, in the Cylc wrapper script:

  • Copy PYTHONPATH into a new var called _PYTHONPATH.
  • Wipe PYTHONPATH for the cylc/rose/whatever execution.
  • In rose, restore PYTHONPATH from _PYTHONPATH for app execution.

For context, here's a common practice:

# flow.cylc
[runtime]                                                               
    [[foo]]                                                             
        pre-script = module load foo    # load some dependencies of our app                            
        script = rose task-run    # <= wipe PYTHONPATH before this                                                                 
# rose-app.conf                                                         
[command]                                                               
default = foo   # <= restore PYTHONPATH before this   

@wxtim
Copy link
Member

wxtim commented Sep 8, 2023

I think that I've done the rose end, but the wrapper script is proving trickier. We don't really want to cache PYTHONPATH every time we run the wrapper script - otherwise it risks stripping out the path we want for apps like cylc message?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could be better Not exactly a bug, but not ideal. investigation question Flag this as a question for the next Cylc project meeting.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants