Transformation configuration and SchedulerConfig update #191

mlange05 · 2023-11-20T15:12:54Z

Note: This change is not backward compatible for our regression test suite! We therefore temporarily point to custom CLOUDSC (ecWAM) branches - the respective branches need to be merged, and the respective commit(s) removed before merging this PR.

This change is part of the wider plan to make the transformation composition more configurable. It primarily brings in the ability to add configuration entries for transformations to the scheduler config file, and then allow the SchedulerConfig to directly instantiate them. This instantiation process allows two things that are important for further refactoring:

Remote instantiation of Transformation classes from any module (even external!)
Resolution of Dimension objects via a custom placeholder notation that allows us to reuse the ones defined in the SchedulerConfig already.

In order to provide a more streamlined configuration file, this PR also removes some backward hand-holding, where we would generate dicts of Dimension/Routine objects with custom handling. Instead, we now utilise the native TOML dict/table nesting to use the name of a Dimension/Routine/Transformation directly form the processed TOML output.

Since this change also means we'll be putting more and more custom logic in SchedulerConfig, I've also moved this to a new sub-package and renamed loki.bulk => loki.batch. @reuterbal, I hope this does not conflict too much for the sgraph rewrite?

And finally, I'm filing this as a draft PR first, to see what breaks in the regression tests, etc. Please feel free to chime in though already, as this is somewhat invasive and might need some careful marshalling.

github-actions · 2023-11-20T15:17:53Z

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/191/index.html

codecov · 2023-11-21T15:12:20Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (37d0033) 92.22% compared to head (ffb6330) 92.23%.

Files	Patch %	Lines
loki/bulk/configure.py	93.44%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #191   +/-   ##
=======================================
  Coverage   92.22%   92.23%           
=======================================
  Files          94       95    +1     
  Lines       16949    16972   +23     
=======================================
+ Hits        15632    15654   +22     
- Misses       1317     1318    +1

Flag	Coverage Δ
lint_rules	`96.22% <ø> (ø)`
loki	`92.20% <95.06%> (+<0.01%)`	⬆️
transformations	`91.45% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mlange05 · 2023-11-22T12:59:49Z

@reuterbal, apologies for the large diff in the tests due to the name restructuring. I can move this into a separate PR if that makes things easier. I'll also file PRs for the corresponding upstream changes in CLODUSC / ecWAM and reference this one here.

reuterbal

First of all, this is a vast improvement in the appearance of the Scheduler config and unlocks an entirely new level of configurability and separation of transformation recipes from target code-specific hand-holding. I'm really pleased about this!

Also, conceptually I don't have much to add - this looks all sound and is in line with what we discussed offline. It needs a rebase, though, and Codecov seems a little confused right now as to what is covered by tests and what not. But I'd like at least additional testing for when the instantiation of transformations from config file entries fails, as I expect this to be a not too rare occurence.

Also, there is now a dependency on the transformations package in the tests of the Loki core library, which I would like to avoid.

loki/tools/files.py

loki/batch/configure.py

tests/test_scheduler.py

mlange05 · 2023-12-08T08:30:14Z

@reuterbal Ok, many thanks for the review (and good catches 😉). I think I've addressed them all now, so this is ready for another look.

reuterbal

Many thanks, this looks very good now and we could include it as is. For 100% gold-star test coverage, there's one combination that could be tested additionally but I'll leave it up to you if you think it's necessary.

However, to reduce the impact on the parallel scheduler rewrite, delaying the bulk->batch rename to a separate, later PR would help. Generally very much in favor of this, though!

And lastly, as discussed, I'll hold off merging until we have rolled a release as this incurs a breaking change.

reuterbal · 2023-12-08T09:17:53Z

tests/test_scheduler.py

+        }
+    }
+    with pytest.raises(ModuleNotFoundError):
+        cfg = SchedulerConfig.from_dict(worse_config)


Just to point this out: For full coverage we would need a worst_config with invalid options provided, that's the only bit currently not covered in the tests.

Hehe, good catch; I'll add it to the rebased push with the removed renaming.

mlange05 · 2023-12-08T10:57:37Z

Ok, great, many thanks @reuterbal ! I've rebased this without the bulk->batch rename (please double-check), and pushed again (including the final test addition). Should be fine now, but waiting for CI to confirm. Please also note that there's a [DO NOT MERGE] commit in there that temporarily re-pointed the branches for regression testing.

reuterbal

Fantastic, many thanks, very happy with this now!

This has currently a conflict, so needs a rebase over latest version of main. I would suggest to remove the [DO NOT MERGE] commit in the process and accept the failing regression tests when merging.
Afterwards, we should see the checks turn green in the pending PRs on ecWAM and CLOUDSC and then merge them:
ecmwf-ifs/ecwam#7
ecmwf-ifs/dwarf-p-cloudsc#61

After both are merged, we can trigger a re-run of the regression tests on main to turn them green again.

This now allos us to encapulate individual Transformation objects, and their individual argument options, in a toml config file.

Since there is more refactoring needed to replace the full incvocation logic, we stick to the ones with custom constructor arguments.

These were previosuly handed down to the transformation constructors, but are not configured dirctly in config.

This avoids depending on the `transformations` sub-package.

reuterbal · 2023-12-19T10:44:19Z

As discussed: Will merge this now and hope for PRs on regression test repositories to turn green, before merging those and re-running regression on Loki.

mlange05 requested review from reuterbal and MichaelSt98 November 20, 2023 15:12

mlange05 mentioned this pull request Nov 20, 2023

Generic approach for transformation specific settings in SchedulerConfig #28

Closed

mlange05 force-pushed the naml-transformation-config branch 6 times, most recently from 0db5dd3 to a6c3659 Compare November 21, 2023 14:48

mlange05 force-pushed the naml-transformation-config branch 3 times, most recently from 4404c25 to 81b88fa Compare November 22, 2023 09:06

mlange05 marked this pull request as ready for review November 22, 2023 12:51

mlange05 force-pushed the naml-transformation-config branch from 81b88fa to 5899bf0 Compare November 22, 2023 12:52

This was referenced Nov 22, 2023

Loki config update ecmwf-ifs/dwarf-p-cloudsc#61

Merged

Loki: Update loki config files to new format ecmwf-ifs/ecwam#7

Merged

reuterbal linked an issue Nov 29, 2023 that may be closed by this pull request

Generic approach for transformation specific settings in SchedulerConfig #28

Closed

reuterbal requested changes Nov 29, 2023

View reviewed changes

mlange05 force-pushed the naml-transformation-config branch 3 times, most recently from 8d40a64 to 21cec88 Compare December 8, 2023 08:26

reuterbal approved these changes Dec 8, 2023

View reviewed changes

mlange05 force-pushed the naml-transformation-config branch from 21cec88 to 1914f34 Compare December 8, 2023 10:54

mlange05 force-pushed the naml-transformation-config branch from 1914f34 to a3e3a46 Compare December 8, 2023 11:00

reuterbal approved these changes Dec 15, 2023

View reviewed changes

mlange05 added 13 commits December 19, 2023 09:59

Batch: Put SchedulerConfig into a separate file

db6ea96

Pylint: Silence warnings about exception rules

02e935f

Tools: Move load_module utility to central tools package

d62445a

Scheduler: Add TransformationConfig to instantiate Trafos from dict

17470fb

This now allos us to encapulate individual Transformation objects, and their individual argument options, in a toml config file.

Scheduler: Require routines and dimensions as list keys

2172508

Scheduler: Let TransformationConfig resolve dimension options

a294753

Scheduler: Add test for dimension-reference trafo config

61527e2

Loki-transform: Using config-based transformations for SCC-CUF

e09f2d8

Since there is more refactoring needed to replace the full incvocation logic, we stick to the ones with custom constructor arguments.

Scheduler: Remove custom config entries for dic2p and derived_types

2e381e8

These were previosuly handed down to the transformation constructors, but are not configured dirctly in config.

Scheduler: Build custom "external" trafo for scheduler config test

046cd96

This avoids depending on the `transformations` sub-package.

SchedulerConfig: Fix docstring and dimension regex

6d28c06

Scheduler: Add tests for bad configuration entries

4954a85

Scheduler: Only support new-style dict-based configurations

ffb6330

mlange05 force-pushed the naml-transformation-config branch from a3e3a46 to ffb6330 Compare December 19, 2023 10:10

reuterbal merged commit 2f5158a into main Dec 19, 2023
8 of 12 checks passed

reuterbal deleted the naml-transformation-config branch December 19, 2023 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transformation configuration and SchedulerConfig update #191

Transformation configuration and SchedulerConfig update #191

mlange05 commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023

codecov bot commented Nov 21, 2023 •

edited

Loading

mlange05 commented Nov 22, 2023

reuterbal left a comment

mlange05 commented Dec 8, 2023

reuterbal left a comment

reuterbal Dec 8, 2023

mlange05 Dec 8, 2023

mlange05 commented Dec 8, 2023

reuterbal left a comment

reuterbal commented Dec 19, 2023

Transformation configuration and SchedulerConfig update #191

Transformation configuration and SchedulerConfig update #191

Conversation

mlange05 commented Nov 20, 2023 • edited Loading

github-actions bot commented Nov 20, 2023

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

mlange05 commented Nov 22, 2023

reuterbal left a comment

Choose a reason for hiding this comment

mlange05 commented Dec 8, 2023

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal Dec 8, 2023

Choose a reason for hiding this comment

mlange05 Dec 8, 2023

Choose a reason for hiding this comment

mlange05 commented Dec 8, 2023

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal commented Dec 19, 2023

mlange05 commented Nov 20, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading