Cylc to ignore PYTHONPATH: Use CYLC_PYTHONPATH for custom Python for Cylc

[wxtim]

Closes #5124

• Remove from sys.path anything in PYTHONPATH.

  • This prevents PYTHONPATH from breaking Cylc.

• Add to sys.path anything in CYLC_PYTHONPATH.

  • This allows us to continue to install Cylc plugins via environment variable.

Check List

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
• Tests this change needs to be manually tested alongside Allow Rose Apps to set PYTHONPATH=_PYTHONPATH before running metomi/rose#2725 . I've tested it with this workflow: tmp.pythonpath.tar.gz
• CHANGES.md entry included if this is a change that can affect users
• Cylc-Doc pull request opened if required at added note about PYTHONPATH & CYLC_PYTHONPATH cylc-doc#665.
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[wxtim]

I've put this back to draft - I have something else to try!

[oliver-sanders]

LGTM; the tests are failing as a few PYTHONPATH variables will need to be updated to CYLC_PYTHONPATH in the test code / workflows.

Manual testing is ok for this, but a basic automated test is possible, put this code in a function, then monkeypatch sys.path.

import sys

def patch_pythonpath():
    ...

def test_patch_pythonpath(monkeypatch):
     monkeypatch.setattr('sys.path', [])
     monkeypatch.setenv('PYTHONPATH', ...)
     monkeypatch.setenv('CYLC_PYTHONPATH', ...)
     patch_pythonpath()
     assert sys.path == [...]

Will need docs too, but should run this by @hjoliver first, explanation:

• People often set PYTHONPATH to add stuff to environments.
• This regularly breaks Cylc/Rose, especially when larger environments are loaded.
• Tomek changed the job script to run user-script in subshells which protects the built-in cylc message usage which made this better, but errors can still arise from within user scripts.
• Rose is much worse affected as you have to activate your environment before running rose {app,task}-run.
• We're currently unable to run the UM test suite at MO due to environment conflicts :(
• The best solution we've come up with is to get Cylc to ignore the PYTHONPATH completely, and add CYLC_PYTHONPATH to cover any legit uses e.g. job-runners, xtriggers, etc.
• Note these use cases would generally be better handled by {pip,conda} install than PYTHONPATH mangling due to this sort of issue, but also lack of entry point functionality, dependency checking, etc.

[wxtim]

Manual testing is ok for this, but a basic automated test is possible, put this code in a function, then monkeypatch sys.path.

I had considered this - I'll do it.

Existing test failures seem to be caused by the print statements I left in the new code.

[wxtim]

@hjoliver Please confirm that you are happy with this approach. 😄

[oliver-sanders]

Hmmm:

ModuleNotFoundError: No module named 'argparse'

https://github.com/cylc/cylc-flow/actions/runs/6315139062/job/17147002345?pr=5727#step:7:19

This is in a task's job.err section where it was raised from cylc/flow/scripts/cylc.py, so presumably a cylc message call from the job script.

I think this suggests that the logic is removing the standard library from sys.path internally?

The previous diff (replacing PYTHONPATH with CYLC_PYTHONPATH) may have suppressed this by bypassing the removal logic?

[oliver-sanders]

Here's how I tested:

$ # create a virtual environment with an incompatible version of psutil
$ virtualenv venv
$ . venv/bin/activate
$ pip install psutil==4

$ # replicate the bug
$ conda activate cylc.dev
$ PYTHONPATH=venv/lib/python3.9/site-packages/ cylc vip generic -N
...
2023-10-16T12:02:07Z ERROR - Workflow shutting down - PluginError: Error in plugin cylc.main_loop.auto_restart
    VersionConflict: (psutil 4.4.2 (/var/tmp/tmp.f8Khmjq2Dp/create/lib/python3.9/site-packages),
    Requirement.parse('psutil>=5.6.0'))
$ # verify the fix
$ git -C ~/cylc-flow checkout wxtim/feature.cache_pythonpath
$ PYTHONPATH=venv/lib/python3.9/site-packages/ cylc vip generic -N
...
 ▪ ■  Cylc Workflow Engine 8.3.0.dev
 ██   Copyright (C) 2008-2023 NIWA
▝▘    & British Crown (Met Office) & Contributors
...
$ # all good!

[oliver-sanders]

We could do with an entry in the references/changes section of cylc-doc explaining the problem and introducing the CYLC_PYTHONPATH workaround.

[MetRonnie]

Does this close #5124?

[MetRonnie]

There is no specification for this behaviour written in the PR which is making it difficult to understand what is supposed to be happening

[wxtim]

@hjoliver , and

There is no specification for this behaviour written in the PR which is making it difficult to understand what is supposed to be happening

Fair Point:
I think this is what is making me somewhat uncertain. I'll ask @oliver-sanders to check that the following is the aim:

1. If the user modifies PYTHONPATH do not allow these items to interfere with Cylc by removing them from sys.path. Leave the environment PYTHONPATH unchanged.
2. If CYLC_PYTHONPATH is set add items in that variable to sys.path for Cylc to use.

What I am now unclear on is whether I want to save the original PYTHONPATH to CYLC_PYTHONPATH - I don't thing I do?

[oliver-sanders]

Yes.

Very simple:

• Remove from sys.path anything in PYTHONPATH.
  • This prevents PYTHONPATH from breaking Cylc.
• Add to sys.path anything in CYLC_PYTHONPATH.
  • This allows us to continue to install Cylc plugins via environment variable.

And nothing else!

What I am now unclear on is whether I want to save the original PYTHONPATH to CYLC_PYTHONPATH

No, there is no reason to do this. We are not changing any environment variables, only sys.path within Cylc Python sessions.

[hjoliver]

🎉

All comments addressed, two other approvals.