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

Introduction of CYLC_PYTHONPATH in cylc-flow 8.1 problematic for spack builds #6532

Open
climbfuji opened this issue Dec 19, 2024 · 6 comments

Comments

@climbfuji
Copy link

climbfuji commented Dec 19, 2024

Problem

Around 8.1, a "feature" was added that strips out PYTHONPATH and replaces it with CYLC_PYTHONPATH in the cylc.py script. This change is highly problematic for spack installations (I assume spack is known here; https://github.com/spack/spack/; if not, let me know).

spack installs every Python package from source in its own directory. That means, every Python dependency that cylc needs lives in its own directory tree and needs its own entry in PYTHONPATH to be found. spack is very good at doing this automatically (via spack load or via auto-generated tcl/tk or lua/lmod modules). The cylc logic to throw away PYTHONPATH and set it to CYLC_PYTHONPATH breaks this, and a user who is trying to run cylc from a spack install gets a "module not found" error for each dependency.

Proposed Solution

I understand that there was a reason for making this change, therefore I am not asking for a reversal, but for a way to make this work with spack. For now, I am simply commenting out the pythonpath_manip function when I am installing cylc-flow with spack: https://github.com/spack/spack/pull/48052/files --> var/spack/repos/builtin/packages/py-cylc-flow/package.py line 55+. This workaround is problematic for two reasons: (1) It may re-introduce some of the problems you were fixing with the introduction of CYLC_PYTHONPATH. (2) The spack cylc then behaves differently from other cylc installations, and the documentation for cylc isn't correct for the spack installs.

I was considering two more options:

  1. spack knows exactly which Python packages cylc-flow needs (depends-on -> type=run). Using spack's setup_run_environment logic, we could populate CYLC_PYTHONPATH automatically with those dependencies (in addition to the usual PYTHONPATH). Loading the cylc module from spack would then set CYLC_PYTHONPATH accordingly and everything should work as-is. The only problem I am seeing with this is that the documentation for cylc-flow assumes that the user can just set ("use") CYLC_PYTHONPATH, it doesn't expect that it may already be set by something else (like, for example, spack). A simple way out here would be to amend the documentation so that it accounts for the fact that CYLC_PYTHONPATH may already be set and the user can add to it if necessary.

  2. Instead of populating CYLC_PYTHONPATH automatically with spack, we could define another environment variable SPACK_CYLC_PYTHONPATH and have spack populate that. Then, spack would "patch" the pythonpath_manip function during the installation of cylc (similar to how I am patching it at the moment in the above mentioned spack-cylc PR) to remove PYTHONPATH from sys.path and add SPACK_CYLC_PYTHONPATH and CYLC_PYTHONPATH. This would completely encapsulate the spack modifications. No changes needed in the cylc-repo or the cylc documentation.

Thoughts?

@hjoliver
Copy link
Member

hjoliver commented Dec 19, 2024

Thanks @climbfuji - note we probably won't be able to consider this till January.

The reason for the change, for reference: #5124

(I think at my site we intend to use (on a new HPC) Spack for anything that requires build from source, but conda for Python packages including Cylc ... but I'll ping a colleague to take a look at what you've said)

@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 2, 2025

Hi 👋.

We are also planing to make more widespread use of Spack, but will continue to deploy Cylc as a system tool in an independent Conda environment.

we could populate CYLC_PYTHONPATH automatically with those dependencies (in addition to the usual PYTHONPATH)

This should do the job.

The Cylc PYTHONPATH issue occurs when people want to make use of tools like Spack (which rely on PYTHONPATH modification) to manage Python environments used within Cylc workflows, e.g:

[runtime]
  [[mytask]]
    script = """
      module load spackenv
      myscript   # should run in spackenv
      cylc message -- done   # should not be broken by spackenv
    """

The cylc message command needs to function, however the module load may have added modules into the PYTHONPATH. PYTHONPATH modification is fragile as new items (even appended on the end) can be incompatible with other items in PYTHONPATH or the base environment resulting in a corrupted stack.

So we would want the Python modules that are part of the Cylc stack to be listed in CYLC_PYTHONPATH, but not any modifications to PYTHONPATH (including shell startup files like .bashrc).

The cylc executable is created when Cylc is installed e.g. by pip (we use a console scripts entry point), this might be a good place to insert the logic.

@climbfuji
Copy link
Author

Here's a quick update from our side. For now, we removed any modification of the cylc logic in the latest updates of the cylc packages in spack. That is, the CYLC_PYTHONPATH logic that was introduced in 8.1.0 remains as is.

Our use case goes a little further than what you describe above: We build two different spack environments: The first environment contains only cylc (cylc-flow, cylc-uiserver, possibly cylc-rose) and is built with gcc. The users who want to run the end-to-end system load only this environment to launch cylc jobs. The second environment can be built with other compilers and provides the environment for running the actual model. The cylc tasks load these environments on a per-task basis.

The easiest way for to make this work without breaking either of the environments is to create a "spack environment view" and a custom module or bash script that sets the PATH variable such that cylc in /path/to/env-view/bin can be found, and that sets CYLC_PYTHONPATH to /path/to/env-view/lib/pythonX.Y/site-packages. It doesn't set LD_LIBRARY_PATH, PYTHONPATH, ...

@oliver-sanders
Copy link
Member

Our use case goes a little further than what you describe above

The first environment contains only Cylc

The second environment ... provides the environment for running the actual model

That is how we work:

  • Cylc is installed in its own environment (we recommend using Conda for this, but Spack should work fine once we get around its reliance on PYTHONPATH).
  • Cylc tasks activate other environments as needed (the tool used to create the environment, e.g. Spack, should not matter here).

The model environment should not pollute the Cylc environment (as this may corrupt it), hence the Cylc PYTHONPATH logic.

The users who want to run the end-to-end system load only this environment to launch cylc jobs

I think this part might be causing the problem. We recommend using a cylc wrapper script that sets (but does not export) the required environment variables for Cylc to function rather than activating the cylc environment directly. This way the Cylc environment is only activated for Cylc commands and is not inherited by local jobs.

E.g, a minimal wrapper script might look like this:

#!/bin/bash
# Cylc wrapper script, put me somewhere in $PATH

exec PATH=... CYLC_PYTHONPATH=.... cylc "$@"

We provide a more advanced wrapper script that works with Conda/Mamba/VirtualEnv/Venv environments. There's some information on this page:

https://cylc.github.io/cylc-doc/latest/html/installation.html#managing-environments

Or you can find the script itself here:

https://github.com/cylc/cylc-flow/blob/master/cylc/flow/etc/cylc

Using a wrapper script:

  • Avoids the need for users to activate the Cylc environment manually (the cylc wrapper script is in $PATH).
  • Allows multiple versions of Cylc to be installed in parallel (use the CYLC_VERSION environment variable to switch).
  • Makes distributed Cylc deployments easier (Cylc needs to run cylc commands on other hosts, environment activation only works for localhost).

@climbfuji
Copy link
Author

The cylc wrapper script is a great idea, thanks!

As for the "but Spack should work fine once we get around its reliance on PYTHONPATH" note, this is where the view comes in. In your spack environment that builds cylc, you set this:

  view:
    cylc:
      root: $SPACK_ENV/view
      select: [^python]
      link: run
      link_type: symlink

This generates a subdirectory view inside your environment and symlinks everything that depends on Python into it (and yes, that id sufficient, because for everything non-Python spack uses rpath). Thus, all that your cylc wrapper script needs to do is to set

PATH=/path/to/view/bin:$PATH
CYLC_PYTHONPATH=/path/to/view/lib/python3.11/site-packages

and you are done.

@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 7, 2025

symlinks everything that depends on Python into it

Linking the packages in sounds great 👍

Thus, all that your cylc wrapper script needs to do is to set

Python should automatically look in the site-packages directory (relative to the bin directory the Python executable is installed into), so this should work even without the CYLC_PYTHONPATH bit (we don't even set $PATH in our wrapper script).


Let us know how you get on.

climbfuji added a commit to JCSDA/spack-stack that referenced this issue Jan 10, 2025
This PR adds a new template cylc-dev to build a special environment for running cylc. Because of the way cylc works, this is best accomplished by creating an environment view and then a cylc wrapper that contains the minimum settings necessary to use cylc from the view's bin directory. See cylc/cylc-flow#6532 for more information. Note that the creation of the cylc wrapper is not part of this PR.

Associated changes:
- To compile cylc-dev on the Ubuntu self-hosted runner, exclude meson from the spack external find call. Spack found a deprecated version [email protected] and despite it being deprecated it tried to use it - with the result that the harfbuzz build failed (needed for cylc-dev). Letting spack build meson (it picks [email protected]) solved the problem.
- Remove the +excel variant from py-pandas, which was something the jedi-tools-env used, but that is no longer needed and that caused build errors (and added several more dependencies).
- Fix a bug in the GitHub actions that would allow the tests to fail silently (replace set +e with set -e).
- To avoid duplicate packages, require py-cython@3 and [email protected]
- With Intel Classic/LLVM, build qt with gcc (has no effect on pre-configured sites, where qt is configured as an external package)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants