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

Frontend: Final removal of OFP frontend #469

Merged
merged 5 commits into from
Jan 10, 2025
Merged

Frontend: Final removal of OFP frontend #469

merged 5 commits into from
Jan 10, 2025

Conversation

mlange05
Copy link
Collaborator

@mlange05 mlange05 commented Jan 8, 2025

o7

@mlange05 mlange05 requested a review from reuterbal January 8, 2025 15:55
Copy link

github-actions bot commented Jan 8, 2025

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

@mlange05 mlange05 force-pushed the naml-remove-ofp branch 2 times, most recently from 4c67a7d to fbfd5af Compare January 9, 2025 08:27
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 (b0a2344) to head (730399b).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #469      +/-   ##
==========================================
+ Coverage   93.35%   96.03%   +2.67%     
==========================================
  Files         223      222       -1     
  Lines       41428    40068    -1360     
==========================================
- Hits        38676    38480     -196     
+ Misses       2752     1588    -1164     
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 96.02% <100.00%> (+2.71%) ⬆️

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 marked this pull request as ready for review January 9, 2025 09:47
@reuterbal reuterbal linked an issue Jan 9, 2025 that may be closed by this pull request
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.

Thanks, looks good.

However, as mentioned in a comment during the deprecation (#406 (comment)) we need to now also remove the routines that were only kept around for OFP. Because they have not had code coverage after the deprecation, this doesn't show up at this point anymore.

Could you take care of the checklist in the above linked comment, please?

@mlange05
Copy link
Collaborator Author

mlange05 commented Jan 9, 2025

Could you take care of the checklist in the above linked comment, please?

Yes, thanks for the pointer, I had forgotten about that. I've removed the main utilities and the test now - the false positives I think we want to address once we expand more low-level unit testing of the interior IR nodes and scoping behaviour.

@reuterbal
Copy link
Collaborator

Great, thanks. Agreed on the false positives, they should stay in.
Any particular reason for keeping the disc-cached utility? https://github.com/ecmwf-ifs/loki/blob/main/loki/tools/files.py#L186-L227
It is currently not being tested: https://app.codecov.io/gh/ecmwf-ifs/loki/pull/469/blob/loki/tools/files.py#L186

@mlange05
Copy link
Collaborator Author

Great, thanks. Agreed on the false positives, they should stay in. Any particular reason for keeping the disc-cached utility?

Ah, good catch, I had removed the option, but not the utility. I've done that now (top commit), and rebased over main.

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, looks good to me now!

Now comes also with a nice uplift of the code coverage to >96%!
(The Github check for codecov is stuck somehow but will override upon merge)

@reuterbal reuterbal added the ready to merge This PR has been approved and is ready to be merged label Jan 10, 2025
@reuterbal reuterbal merged commit 7668989 into main Jan 10, 2025
13 checks passed
@reuterbal reuterbal deleted the naml-remove-ofp branch January 10, 2025 13:55
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.

Remove OFP frontend
2 participants