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

Add scoping support to gathering events #207

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Conversation

fsimonis
Copy link
Member

@fsimonis fsimonis commented Oct 25, 2024

Main changes of this PR

This PR changes gatherstats to use scopes for extracting timings from the profiling data, making it robust against the introduction of additional scopes in between.

Related to precice/precice#2066

Author's checklist

  • I used the pre-commit hook and used pre-commit run --all to apply all available hooks.
  • I added a test to cover the proposed changes in our test suite.
  • I updated the documentation in docs/README.md.
  • I added a changelog entry in ./changelog-entries/ (create if necessary).
  • I updated potential breaking changes in the tutorial precice/tutorials/aste-turbine.

@fsimonis fsimonis self-assigned this Oct 25, 2024
@davidscn
Copy link
Member

Ah that's nice. Did you check the result? Since you are already taking a look here: we have the script as part of our CI. Could we check as part of our CI that the gathered stats are 'correct' or within bounds? This would make testing way easier, as the script might not complain/proceed, when a timing is 'not present'. In the current CI, this would be a pass, but insufficient from a functionality POV.

@fsimonis
Copy link
Member Author

fsimonis commented Oct 25, 2024

Ah that's nice. Did you check the result?

Results are now identical to the before.
I checked with preCICE precice/precice#2066

Since you are already taking a look here: we have the script as part of our CI. Could we check as part of our CI that the gathered stats are 'correct' or within bounds? This would make testing way easier, as the script might not complain/proceed, when a timing is 'not present'. In the current CI, this would be a pass, but insufficient from a functionality POV.

A CI python script that checks if the timings contain possible values (as in >0) would be a good first step. But checking runtime values is always a problem without controlled hardware.

@davidscn
Copy link
Member

It would be sufficient to test that we have a "reasonable" value. Examples: a lately introduced 'sync' event gave me some zeros. The renaming of events gives nothing. The event scoping runs into the risk of a similar 'nothing' value (putting aside that you tested it for now).

@fsimonis
Copy link
Member Author

Right. Opening an issue regarding this

@fsimonis fsimonis merged commit 96b8431 into develop Oct 25, 2024
3 checks passed
@fsimonis fsimonis deleted the prepare-for-scoping branch October 25, 2024 11:14
@davidscn
Copy link
Member

The issue is insufficient to ensure functionality for upcoming changes though

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