Skip to content

(closes #2981) Fixes for profile trans without the updates#3009

Merged
sergisiso merged 14 commits intomasterfrom
2981_exit_profiling
Jun 16, 2025
Merged

(closes #2981) Fixes for profile trans without the updates#3009
sergisiso merged 14 commits intomasterfrom
2981_exit_profiling

Conversation

@LonelyCat124
Copy link
Collaborator

This is the small PR with just the functionality changes from #3004 .

@codecov
Copy link

codecov bot commented May 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.91%. Comparing base (3b84d6b) to head (b025c3c).
Report is 15 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3009   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files         365      365           
  Lines       51930    51956   +26     
=======================================
+ Hits        51886    51912   +26     
  Misses         44       44           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@LonelyCat124 See question below

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@LonelyCat124 See comment below

@LonelyCat124
Copy link
Collaborator Author

@sergisiso ready for another look - only thing I'm not sure about is using debug_string as opposed to something neater.

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

Thanks @LonelyCat124 . This is getting closer, see inline comments. Is the PR still "towards" and not "closes"?

@LonelyCat124
Copy link
Collaborator Author

I think its towards as I'd like to leave the issue open until #3004 is ready to deal with the rest of the options updates.

@LonelyCat124
Copy link
Collaborator Author

@sergisiso Ready for another look.

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@LonelyCat124 See some more comments below

@LonelyCat124
Copy link
Collaborator Author

I think this is ready for review again @sergisiso

@arporter
Copy link
Member

A quick note of encouragement - this PR fixes the issues I saw when attempting to profile NEMOv5 with Chris' sea-ice refactoring :-)

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

Thanks for doing the changes @LonelyCat124, I like this implementation with the encapsulated "has_potential_control_flow_jump" much more, this is ready to merge.

I will close the issue and rename the other PR, as this already fully fixes the issue.

@sergisiso sergisiso changed the title (#Towards 2981) Fixes for profile trans without the updates (closes #2981) Fixes for profile trans without the updates Jun 16, 2025
@sergisiso sergisiso merged commit a7dcf48 into master Jun 16, 2025
12 checks passed
@sergisiso sergisiso deleted the 2981_exit_profiling branch June 16, 2025 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants