NOAA-GFDL / fre-cli

Python-based command line interface for FRE (FMS Runtime Environment) to compile and run FMS-based models and post-process their output.
GNU Lesser General Public License v3.0
3 stars 11 forks source link

Add cylc to conda dependencies #46

Closed ceblanton closed 7 months ago

ceblanton commented 7 months ago

Various fre pp tools use cylc, and currently it is module loaded at GFDL.

This change would allow the fre pp tools to work at non-GFDL sites and generally easily, at the cost of a larger conda install.

ceblanton commented 7 months ago

I think we want this, for gaea and generic site use primarily (and also it eases the GFDL installation and upgrades), but it will make the conda installation bigger and slower. It's already slow, though.

@singhd789 and @Ciheim's containers conda install cylc-flow, cylc-rose, and metomi-rose, so those 3 are definitely needed.

cylc-uiserver, though, is also in the "module load cylc" environment and needed for "cylc gui". Maybe we will want to add it later, though it seems un-cli like.

@bcc2761 @chanwilson

bcc2761 commented 7 months ago

sounds good to me; we can always remove if we find a better solution

singhd789 commented 7 months ago

In containerized post-processing world, as you mentioned Chris, we do have cylc installed in the container. If we eventually get to just a base cylc container instead of the full pp workflow, that would eliminate the installation in the fre-cli, replacing it with a container usage instead, right? Just a thought.

ceblanton commented 7 months ago

In containerized post-processing world, as you mentioned Chris, we do have cylc installed in the container. If we eventually get to just a base cylc container instead of the full pp workflow, that would eliminate the installation in the fre-cli, replacing it with a container usage instead, right? Just a thought.

I have to confess I'm a bit behind on the container granularity and how the fre-cli and other dependencies interact.

Is there a disadvantage to having cylc in the fre-cli envionment? It would be useful for GFDL, gaea, and generic use (without containers), but may not be useful for the containers themselves.

singhd789 commented 7 months ago

In containerized post-processing world, as you mentioned Chris, we do have cylc installed in the container. If we eventually get to just a base cylc container instead of the full pp workflow, that would eliminate the installation in the fre-cli, replacing it with a container usage instead, right? Just a thought.

I have to confess I'm a bit behind on the container granularity and how the fre-cli and other dependencies interact.

Is there a disadvantage to having cylc in the fre-cli envionment? It would be useful for GFDL, gaea, and generic use (without containers), but may not be useful for the containers themselves.

It is a conda based container and in the dockerfile, it creates a conda environment (https://github.com/NOAA-GFDL/HPC-ME/blob/86af3f664e3f07eb1df09eeb3eadef6dbf7db075/ppp/ppp-Dockerfile#L17) and uses the environment yaml (https://github.com/NOAA-GFDL/HPC-ME/blob/main/ppp/cylc-flow-tools.yaml) to install tools needed for the workflow. @Ciheim recently added the fre-cli installation into the yaml as well. So the fre-cli operates in this environment where cylc tools are already installed.

It's not a big disadvantage, and is good for a non-container use. I just had the thought that it would help the slowness of the install since it's already in the container. Since the container applies to the whole workflow at the moment though, this was a possible "in the future" thought for if we just use a cylc container.

ceblanton commented 7 months ago

The conda build tests are not working, but I can't make sense of the log output. @bcc2761 could you take a look sometime to see if you see the problem?

The strange part is that while the most recent build failed (https://github.com/NOAA-GFDL/fre-cli/actions/runs/8330212607),

a previous one that had the duplicate "metomi-rose" dependencies seemed to have worked (https://github.com/NOAA-GFDL/fre-cli/actions/runs/8299084818)

singhd789 commented 7 months ago

The conda build tests are not working, but I can't make sense of the log output. @bcc2761 could you take a look sometime to see if you see the problem?

The strange part is that while the most recent build failed (https://github.com/NOAA-GFDL/fre-cli/actions/runs/8330212607),

a previous one that had the duplicate "metomi-rose" dependencies seemed to have worked (https://github.com/NOAA-GFDL/fre-cli/actions/runs/8299084818)

Maybe it needs the conda forge channel? - like here https://anaconda.org/conda-forge/metomi-rose

ceblanton commented 7 months ago

I can't disagree with you based on the evidence, but shouldn't conda-forge be available since we added in the "channels" section just above (line 16)

channels:
    - defaults
    - conda-forge
    - noaa-gfdl

I've wondered whether the prereq packages must or should include the channel.

bcc2761 commented 7 months ago

Yeah this one's interesting. https://github.com/mamba-org/mamba/issues/1583 complains about the same issue, but we're not using mamba... I added a conda update conda-build here (https://github.com/NOAA-GFDL/fre-cli/actions/runs/8331516681/job/22798591714?pr=49) and the build succeeded but I'm just confused because the runner should be using the latest version of conda-build regardless; so I feel like this isn't the actual reason behind the fails

bcc2761 commented 7 months ago

^ Nevermind that, it should have failed since that action run didn't even use my PR change but instead used the current workflow script. I have 0 idea what the cause is but I'm gonna look into it more

bcc2761 commented 7 months ago

@ceblanton I just told Dana that I think these new dependencies are unrelated to the problem we're having, as this workflow also failed with the same issue, before these dependencies were added (https://github.com/NOAA-GFDL/fre-cli/actions/runs/8329589424). Trying another test run right now just to see if it's possibly fixed itself and was maybe an issue on Conda's end

singhd789 commented 7 months ago

Seems to have passed, I'll merge it in