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

Develop manual performance regression checking #256

Closed
MarionBWeinzierl opened this issue Jul 4, 2024 · 7 comments · Fixed by #356
Closed

Develop manual performance regression checking #256

MarionBWeinzierl opened this issue Jul 4, 2024 · 7 comments · Fixed by #356
Assignees
Labels
enhancement New feature or request

Comments

@MarionBWeinzierl
Copy link
Collaborator

MarionBWeinzierl commented Jul 4, 2024

After #242 , it was decided to disable the automatic profiling in the GH Actions. Manual profiling can still be done running poetry run /usr/bin/time -v pytest -m "profiling" --profile-svg.

What we want now is the ability to compare the results of two profiling runs of different commits in order to check whether performance has deteriorated (or improved) through code changes.

@MarionBWeinzierl MarionBWeinzierl converted this from a draft issue Jul 4, 2024
@MarionBWeinzierl MarionBWeinzierl added bug Something isn't working enhancement New feature or request labels Jul 4, 2024
@MarionBWeinzierl MarionBWeinzierl self-assigned this Oct 17, 2024
@MarionBWeinzierl MarionBWeinzierl removed the bug Something isn't working label Oct 17, 2024
@MarionBWeinzierl MarionBWeinzierl changed the title Switch to manual profiling Develop manual performance regression checking Oct 17, 2024
@MarionBWeinzierl
Copy link
Collaborator Author

Possible solution: Use git-worktree to handle running the profiling code on multiple commits for comparison.

@davidorme
Copy link
Collaborator

Is this because we don't want the profiling code and data to change when moving between commits? We could move the profiler functionality into its own repo to isolate it? Then we would 'just' need to give a path to the pyrealm repo and the two commit hashes to compare?

@MarionBWeinzierl
Copy link
Collaborator Author

Git worktree actually works quite nicely for this, so I am happy with that.

The only issue I am having at the moment is that, while the signature for PModel has changed a while ago, the profiling code has not been adapted at the same time. Therefore, the profiling falls over for commits from that time period. I am thinking about how to elegantly tackle this. Changing the profiling code in place, or copying over/using the new (corrected) version seem clumsy, but I struggle think of something better atm.

@davidorme
Copy link
Collaborator

davidorme commented Oct 23, 2024

Ack - that is ugly. I think I'm happy to accept that the new profiling only works from "now" but I don't know how we define now in the implementation.

More broadly, do we need a mechanism to check that the profiling code continues to run as the code base changes? I mean, we have one, in that they are run by pytest but we don't want them run on every check and we presumably don't want them run on the GitHub actions...

I guess what we could do is set those tests up in pytest with an minimal data input that will always run and validate the profiling code runs but do it fast, and then the manual profiling command somehow switches in a different data input that provides a suitably heavy test load. Maybe by setting environment variables of paths to large datasets for full tests and have the profiling tests check for those before falling back to the light load?

@MarionBWeinzierl
Copy link
Collaborator Author

I might be overcomplicating this, but wondering whether ReFrame, which is quite popular for HPC applications and system tests, would be another option. But maybe that's something to consider as potential next step. And maybe it's too powerful for this.

@MarionBWeinzierl
Copy link
Collaborator Author

I created #345 to fix the signature issue, and implement a GH Action to prevent something like that from happening again for the profiling code.

@MarionBWeinzierl
Copy link
Collaborator Author

MarionBWeinzierl commented Nov 12, 2024

On the connected branch I created some proof-of concept code for a simplified regression testing (after trying to use the existing benchmarking code, which fell over in a couple of places for me, and might be more sophisticated than we actually need it at this point).

See #356 and particularly 20359e0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants