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

ci: Reinstantiate CI files #75

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from
Open

ci: Reinstantiate CI files #75

wants to merge 26 commits into from

Conversation

JesperDramsch
Copy link
Member

@JesperDramsch JesperDramsch commented Jan 10, 2025

This PR adds the following workflows:

  • Public Label PR
  • Conventional Commit PR titles
  • PR labeller for convenience
  • PR Pre-commit and testing

These depend on merge of:

@FussyDuck
Copy link

FussyDuck commented Jan 10, 2025

CLA assistant check
All committers have signed the CLA.

@MeraX
Copy link
Contributor

MeraX commented Jan 13, 2025

Hi Jesper, I went through the yaml files. Most of them look uncontroversial to me. However, the reusable workflows aren't happy with their integration: reusable workflows should be referenced at the top-level jobs.*.uses' key, not within steps` https://github.com/ecmwf/anemoi-core/actions/runs/12710997502

- name: Detect changed packages
id: changed-packages
run: |
CHANGED_FILES=$(git diff --name-only ${{ github.event.before }} ${{ github.sha }})
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to understand this line. As I understand it, it checks if there are changes between the commit pushed to develop (${{ github.sha }} and the previous HEAD of develop ${{ github.event.before }}. Subsequently, you install any of the three packages from the repository, if there were and changes and use PyPI otherwise.

Does this assume, that the previous HEAD of develop is always equal to the version on PyPI?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried not to make implicit assumptions here. I thought it makes most sense to try and see if there are any changes compared to the previous version on develop and not necessarily the released version.

@MeraX
Copy link
Contributor

MeraX commented Jan 13, 2025

And another question: The Github Summary above reports "All checks have passed", even though the python-pull-request.yml is red. Do you consider making python-pull-request.yml mandatory once you PR is merged?
image

@MeraX
Copy link
Contributor

MeraX commented Jan 13, 2025

Another thing that came to my mind: You are installing the three -core packages in individual steps. This might lead to a different resolution of package versions than installing them all at once: pip install -e graphs/[tests] -e training/[tests] -e models/[tests]. In particular, the current version would first install anemoi-graphs and anemoi-models from PyPI, as both are dependencies of ./training. In subsequent steps, both packages are overwritten with local versions. However, dependencies might still be installed in versions required in the PyPI packages and not as required by the local packages.

@JesperDramsch
Copy link
Member Author

Hi Jesper, I went through the yaml files. Most of them look uncontroversial to me. However, the reusable workflows aren't happy with their integration: reusable workflows should be referenced at the top-level jobs.*.uses' key, not within steps` https://github.com/ecmwf/anemoi-core/actions/runs/12710997502

Oh that's a problem. Wasn't aware of that limitation.

@JesperDramsch
Copy link
Member Author

This now runs the tests of changed code against:

  1. Currently released code on PyPI (local-changes)

This mimics the way all other code in Anemoi currently runs, specifically against the released versions of other packages.

  1. All other develops

This ensures that the code will stay up to date with coming changes on develop.

Current limitations

This can mean that if there are changes on develop that are not backwards compatible, but also aren't released yet, then some of the tests will fail.

I believe it's important to test both against released and current code on develop. so I added the label skip-ci-release-tests which can be applied to skip tests against pyPI releases, when we know there was a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Under Review
Development

Successfully merging this pull request may close these issues.

3 participants