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-transform: Remove custom entry point options #465

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

mlange05
Copy link
Collaborator

@mlange05 mlange05 commented Dec 24, 2024

Note: This sits on top of PR #464 and requires CLOUDSC CLOUDSC PR 106 to be merged first.

Note: For testing only at this stage.

Following the conversion to config-file based pipelines, this PR now removes all the custom entry point options under loki-transform.py convert and their associated use in CMake macros. It also removes the now unused loki_transform_convert entry point.

@mlange05 mlange05 requested a review from reuterbal December 24, 2024 05:09
@mlange05 mlange05 changed the title Naml remove custom entry points Loki-transform: Remove custom entry point options Dec 24, 2024
Copy link

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

@mlange05 mlange05 force-pushed the naml-remove-custom-entry-points branch 2 times, most recently from bc4ac13 to 2f21edd Compare January 9, 2025 18:24
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.03%. Comparing base (304921f) to head (7dfefc4).
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #465   +/-   ##
=======================================
  Coverage   96.03%   96.03%           
=======================================
  Files         226      226           
  Lines       40581    40587    +6     
=======================================
+ Hits        38972    38978    +6     
  Misses       1609     1609           
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 96.02% <ø> (+<0.01%) ⬆️

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 mlange05 force-pushed the naml-remove-custom-entry-points branch from c81485b to 9e9169b Compare January 15, 2025 08:59
@mlange05 mlange05 force-pushed the naml-remove-custom-entry-points branch from 9e9169b to e3987b9 Compare January 15, 2025 13:30
@mlange05
Copy link
Collaborator Author

This should now be ready, but still relies on some downstream dev branches. I'll put this into Review mode, once these are merged and I've removed the respective re-pointing commits.

@mlange05 mlange05 force-pushed the naml-remove-custom-entry-points branch from e3987b9 to 50a08b5 Compare January 16, 2025 15:12
@mlange05 mlange05 marked this pull request as ready for review January 16, 2025 15:26
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.

This is very pleasing to see!

In addition to removing the processing of the CMake options we should directly remove them from the corresponding function interfaces - this will make sure to hard fail with an error instead of silently ignoring deprecated options, which I think is better at this stage.

@@ -291,26 +291,6 @@ function( loki_transform_target )
list( APPEND _TRANSFORM_OPTIONS CPP )
endif()

if( _PAR_T_INLINE_MEMBERS )
Copy link
Collaborator

Choose a reason for hiding this comment

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

With these removed, we should now also remove the corresponding arguments from the CMake function - that will make sure that any options that have been specified are not silently ignored but do actually raise an error in CMake directly. For that, ll. 226-231 need to look like this:

    set( options NO_PLAN_SOURCEDIR COPY_UNMODIFIED CPP CPP_PLAN )
    set( single_value_args TARGET COMMAND MODE FRONTEND CONFIG PLAN )
    set( multi_value_args SOURCES HEADERS DEFINITIONS INCLUDES )

Please also update the docstring to match the reduced interface.

@@ -84,41 +84,6 @@ macro( _loki_transform_parse_options )
list( APPEND _ARGS --cpp )
endif()

if( _PAR_DATA_OFFLOAD )
Copy link
Collaborator

Choose a reason for hiding this comment

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

With these removed, the argument definition to loki_transform (ll. 48-57 in loki_transform.cmake) should be updated accordingly:

    set( options CPP )
    set( oneValueArgs COMMAND MODE FRONTEND CONFIG BUILDDIR )
    set( multiValueArgs OUTPUT DEPENDS SOURCES HEADERS INCLUDES DEFINITIONS OMNI_INCLUDE XMOD )

Please also update the docstring of loki_transform accordingly.

Also, since we only have the CPP option left now, it might be worthwhile to just remove this macro and instead inline the remaining 3 lines of code into loki_transform and loki_transform_plan directly

if plan_file is not None:
msg = '[Loki] ERROR: Plan mode requires a pipeline definition in the config file.\n'
if not mode in config.pipelines:
msg = '[Loki] ERROR: Pipeline or transformation mode not found in config file.\n'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be helpful to mention the specified mode here, to spot typos quickly:

Suggested change
msg = '[Loki] ERROR: Pipeline or transformation mode not found in config file.\n'
msg = f'[Loki] ERROR: Pipeline or transformation mode "{mode}" not found in config file.\n'


if plan_file is not None:
msg = '[Loki] ERROR: Plan mode requires a pipeline definition in the config file.\n'
if not mode in config.pipelines:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pet peeve, I find this much more readable:

Suggested change
if not mode in config.pipelines:
if mode not in config.pipelines:

@mlange05
Copy link
Collaborator Author

@reuterbal Indeed, very satisfying! 😉 Thanks for the pointers, I think it's ready for another look.

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.

Many thanks!

@reuterbal reuterbal added the ready to merge This PR has been approved and is ready to be merged label Jan 17, 2025
@reuterbal reuterbal merged commit 2c0e8bf into main Jan 17, 2025
13 checks passed
@reuterbal reuterbal deleted the naml-remove-custom-entry-points branch January 17, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants