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

Loki transformations from config file #94

Merged
merged 15 commits into from
Nov 4, 2024
Merged

Conversation

mlange05
Copy link
Collaborator

@mlange05 mlange05 commented Aug 14, 2024

This change updates Loki to the recently release version v0.2.6, updates the python module paths for the SCC-CUF family transformations and adds config entries for the three main SCC-class pipelines to the config. This effectively now triggers all Loki transformations from config file, thus allowing the eventual removal of some of the custom entry-point logic in loki-transform.py convert.

Note: I previously tried to piggyy-back this with the SCCRawStack transformation (PR #86), but that created weird crossfire for GCC builds. So one step at a time now...

Edit: Another update on this - the actual use of full-scale pipelines was actually trickier than I thought, but is now complete. This PR now switches all except the C-transpilation variants to use composed pipelines from config-files.

Integration note: The top-level commit repoints the Loki version to a dev branch, which needs removing or repointing to master once the requirements have been merged!

@mlange05 mlange05 requested a review from reuterbal August 14, 2024 15:05
@mlange05 mlange05 marked this pull request as draft August 19, 2024 10:18
@mlange05 mlange05 force-pushed the naml-loki-config-from-file branch 3 times, most recently from f73f1b4 to a1e27b2 Compare August 24, 2024 05:00
@mlange05 mlange05 force-pushed the naml-loki-config-from-file branch 3 times, most recently from ab48012 to 3db9576 Compare August 26, 2024 07:52
@mlange05 mlange05 marked this pull request as ready for review August 26, 2024 07:58
@mlange05
Copy link
Collaborator Author

@reuterbal This was trickier than originally envisaged, but it now seems to work for all custom, SCC and SCC-CUF pipelines (C-transpilation is left for another PR). Please have a look and see if this style is suitable for future developments.

There's also a weird recurring failure in the PyIface variant that seems unrelated, but odd. I'd appreciate any help or insights into potential CI gremlins if you have a minute to spare.

@mlange05 mlange05 force-pushed the naml-loki-config-from-file branch 2 times, most recently from a11396f to 55e9c9c Compare August 26, 2024 09:27
@mlange05 mlange05 force-pushed the naml-loki-config-from-file branch from 55e9c9c to 818d0b5 Compare August 26, 2024 10:04
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Apologies for the delay. This looks fantastic!

I would keep the Loki version in the bundle fixed (I realize that this was likely not possible at the time the PR was opened because there wasn't a release for that functionality, yet) to have CLOUDSC reference a stable version.

Otherwise it might make sense to add some more details in the config file simply to be able to point others to it as a reference.

bundle.yml Outdated
@@ -35,7 +35,7 @@ projects :

- loki :
git : https://github.com/ecmwf-ifs/loki
version : v0.2.1
version : main
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should fix a version here again, to make sure that CLOUDSC itself is stable - regression testing with latest main will be triggered from Loki side

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I finally got Loki v0.2.7 to work with this and pinned this version now. Anything before that will be able to compile the SCC-CUF transformations (enabled via --with-cuda), because of a Loki issue in v0.2.6.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is more or less the public reference of how transformation pipelines can be defined in config files, we should maybe expend a somewhat detailed comment at the top of the file to explain the concept. Maybe something that at least touches on the fundamental building blocks:

  • Definition of Scheduler behaviour ([default] - this should really be renamed :))
  • Definition of entry points/item-specific config ([routines])
  • Definition of transformation configs ([transformations]) that represent processing steps that will be used in one or multiple pipelines, and that each of them receives a name
  • Definition of processing pipelines ([pipelines]) that combine multiple transformations into a processing sequence

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good point. I pushed a slightly more verbose explanation to the main loki config.

@mlange05 mlange05 force-pushed the naml-loki-config-from-file branch from 9874f7f to 0088d72 Compare October 28, 2024 12:17
…-fixups

loki_transform for CUF variants, bundle.yml fixup
@reuterbal reuterbal merged commit 83d22d5 into develop Nov 4, 2024
18 checks passed
@reuterbal reuterbal deleted the naml-loki-config-from-file branch November 4, 2024 20:31
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

Successfully merging this pull request may close these issues.

2 participants