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

CMake refactoring for loki_transform #166

Conversation

reuterbal
Copy link
Collaborator

This refactors the CMake integration of Loki. As this is intended to incorporate the changes in #149 and resolve the outstanding issues in there, this is filed as a PR against that branch.

The following changes are applied:

  • A single entry point loki_transform is now the default invocation of loki-transform.py, replacing loki_transform_convert and loki_transform_command. This is also used by loki_transform_target to apply the actual transformations.
  • The various utility macros to translate arguments of the CMake functions to command-line arguments for loki-transform.py have been consolidated into two routines to parse options and arguments. This is possible because validating arguments is now left purely to the Python layer, i.e., the loki-transform.py script. However, the CMake functions calling these translation macros still only accept the explicitly provided list of options and arguments, thus we don't risk silently ignoring options, e.g. when they have a typo or use an outdated interface.
  • loki_transform_convert is marked as deprecated but retained as an entry point for backwards-compatibility, since this is used by regression tests. Once the regression test repositories have been brought forward, this entry point can be completely dropped
  • loki_transform_plan will remain as a separate entry point since this invokes loki at configuration (and not at build time).
  • loki_transform_transpile remains as well, since the interface in the CLI script is very different from convert. However, I have applied the same renaming of options there to align this further, in preparation for future integration into the convert command.

Overall, the loki_transform.cmake script has shrunk by almost 25% (mostly by moving purely utility routines to a separate file), but can be further reduced when the deprecated convert and transpile entry points are removed.

Tested locally for CLOUDSC, waiting for CI now to confirm this works also on ecWAM.
@mlange05: Please test this for ecphys!

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

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

@reuterbal
Copy link
Collaborator Author

PS: This also fixes an unrelated bug related to the member inliner, which wasn't applied even when --inline-members was specified. This was due to the fact that the option wasn't forwarded to the SCCBaseTransformation.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #166 (d3f80bc) into naml-convert-argument-array-shape (9cf5f51) will not change coverage.
The diff coverage is n/a.

@@                        Coverage Diff                         @@
##           naml-convert-argument-array-shape     #166   +/-   ##
==================================================================
  Coverage                              92.10%   92.10%           
==================================================================
  Files                                     90       90           
  Lines                                  16628    16628           
==================================================================
  Hits                                   15315    15315           
  Misses                                  1313     1313           
Flag Coverage Δ
lint_rules 96.22% <ø> (ø)
loki 92.08% <ø> (ø)
transformations 91.25% <ø> (ø)

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

Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

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

This looks fantastic - many, many thanks for taking this over! I've flagged one showstopper that breaks regression (yes, we need to start using the "target" entry point in CLOUDSC!); and flagged a minor outdated comment. Should be good once the target mode is fixes and regression runs!

# )
#
# Call ``loki-transform.py convert ...`` with the provided arguments.
# Call ``loki-transform.py <ecphys|...> ...`` with the provided arguments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think ecphys is basically deprecated now too, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good spot. Yeah, ecphys is already gone. Will change to convert.

)

endfunction()

##############################################################################

macro( _loki_transform_parse_target_args _func_name )
Copy link
Collaborator

Choose a reason for hiding this comment

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

EC-phys regression tells me that there's still a dangling call to this, but it seems to be removed entirely. Was this intended to be moved to helpers, or is it entirely redundant now? In the latter case loki_transform_target needs a small update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for testing and flagging! Yes, we need regression testing of this entry point asap. Please have another go, I have removed the dangling call which shouldn't have provided more than argument checking in this instance.

Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

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

Ok, again, many, many thanks for this (beyond initial scope, but dearly needed!). ECphys regression ran fine now, so GTG from me. :shipit:

@mlange05 mlange05 merged commit 767dc91 into naml-convert-argument-array-shape Oct 8, 2023
12 checks passed
@mlange05 mlange05 deleted the nabr-naml-convert-argument-array-shape branch October 8, 2023 08:19
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