Skip to content

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Oct 1, 2025

An attempt to ensure that the conda-environment.yml file is kept in sync with the pip config file(s).

For context, see conda-forge/cylc-flow-feedstock#116 (comment)

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 are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@oliver-sanders
Copy link
Member Author

That did the trick, the conda builds failed with this error:

ERROR conda.cli.main_run:execute(127): `conda run pip check` failed. (See above for error)
cylc-flow 8.6.0 has requirement protobuf<8,>=5, but you have protobuf 4.24.4.

Reflecting the incompatibility between the Conda and pip metadata.

This will help in detecting cases where pip deps have been updated, but conda deps haven't, but won't catch all cases.

@oliver-sanders oliver-sanders added this to the 8.6.x milestone Oct 1, 2025
@oliver-sanders oliver-sanders self-assigned this Oct 1, 2025
@oliver-sanders oliver-sanders marked this pull request as ready for review October 1, 2025 15:02
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Oct 7, 2025

Note, the test will fail until the dependency is fixed!

Could also fix the dep on this PR, but would like reviewers to see the failure first.

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, makes sense to me to fix the dependency in this PR

- name: check cylc installation
shell: bash -el {0}
run: |
# make sure Cylc is functioning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I noticed when testing locally is that conda seems pretty keen on installing Python 3.12 even when the current is 3.14. Can we check that 3.14 is not being overridden?

Suggested change
# make sure Cylc is functioning
conda run -n test python --version
# make sure Cylc is functioning

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would just be testing the conda-incubator/setup-miniconda@v3 action?

Copy link
Member

@MetRonnie MetRonnie Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned the python-version we specify is not working (e.g. for 3 it looks like we might be getting 3.12: https://github.com/cylc/cylc-flow/actions/runs/18500486273/job/52715670153?pr=7013#step:4:1438

Possibly this issue: conda-incubator/setup-miniconda#114

Copy link
Member Author

@oliver-sanders oliver-sanders Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

We could hard pin the python version at the Conda level (i.e add python = <version> into the conda environment file) which should be as safe as the Conda environment itself (if that dependency brokering is broken, all bets are off).

Copy link
Member Author

@oliver-sanders oliver-sanders Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the above suggestion in c91c402

Unfold the last section in the "build Conda env" step to reveal the version installed.

Copy link
Member Author

@oliver-sanders oliver-sanders Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I we want to force our actions (including this one) to run with the latest Python, we'll have to explicitly state it.

But that's a right maintenance pain as we'll have to keep updating it :(

  • Using an action to extract the version is possible, but ugly, but at least automated.
  • Configuring it in a file is still a maintenance pain, but at least a centralised one.
  • Setting it as a repository secret/variable is a maintenance pain and non-obvious, but at least centralised.

Orthogonal to this new check either way, but probs something we should look into soon to help spot leading edge issues sooner as current tests ain't gonna do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, I was thinking of actions/setup-python which does this effortlessly. I guess updating the latest one once a year isn't too bad... But at present there is no point having a separate 3.12 and 3 in the matrix

Copy link
Member Author

@oliver-sanders oliver-sanders Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With setup-python, 3.x always picks the latest Python version for the operating system, not necessarily the latest Python.

With Conda, 3 always picks the latest Python compatible with the preferable solve of the environment, not necessarily the latest Python.

Neither is perfect, but setup-python is more likely to give the preferred answer more of the time.

Specifying 3 isn't "pointless", once the dependency issue that this PR is identifying has been resolved, it will pick up 3.14 as intended.

Copy link
Member

@MetRonnie MetRonnie Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh it was choosing 3.12 because of the old protobuf pin, got it.

Still, it is useful to have a conda run -n test python --version or better yet conda list -n test step for debugging purposes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It isn't ideal that this can happen, am mulling possible workarounds for future...

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Oct 15, 2025

You've seen the test fail as intended, so I've pushed the dependency fix in bfda2ee

(cancelling the other checks)

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Oct 15, 2025

Et voila, tests pass and the Python is 3.14

@MetRonnie MetRonnie modified the milestones: 8.6.x, 8.6.1 Oct 16, 2025
@MetRonnie MetRonnie added the infrastructure GH Actions, Codecov etc. label Oct 16, 2025
@MetRonnie MetRonnie modified the milestones: 8.6.1, 8.7.0 Oct 16, 2025
@MetRonnie MetRonnie merged commit 63b8564 into cylc:master Oct 16, 2025
2 of 23 checks passed
@oliver-sanders oliver-sanders deleted the packaging-tests++ branch October 16, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure GH Actions, Codecov etc. small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants