ImperialCollegeLondon / pyrealm

Development of the pyrealm package, providing an integrated toolbox for modelling plant productivity, growth and demography using Python.
https://pyrealm.readthedocs.io/
MIT License
16 stars 6 forks source link

Fixing profiling and comparing profiling implementations #225

Closed davidorme closed 2 months ago

davidorme commented 2 months ago

Description

The issue #224 discusses a couple of ways to rethink when and how profiling is run. This PR implements two ways, so we can see how they pan out in parallel.

So:

pyrealm_profiling_after_push.yaml: An approved PR is merged, the profiling and benchmarking runs and a second PR is created to add the updates (hopefully with a warning on failed tests).

πŸ‘ Only runs once. πŸ‘Ž Requires an extra PR, failing code is now in develop.
πŸ‘Ž 🀯 Oh god, and I suppose it is going to start of another profiling run when it gets merged, which will... etc etc.

pyrealm_profiling_on_approve.yaml: An approved PR is merged, the profiling and benchmarking runs and a second PR is created to add the updates (hopefully with a warning on failed tests).

πŸ‘ Stops failing code making it into develop without fixing/justification, single PR. πŸ‘Ž Might run several times.

Fixes #224 (maybe)

Type of change

Key checklist

Further checks

davidorme commented 2 months ago

pre-commit.ci run

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.23%. Comparing base (89f8ff1) to head (f810269). Report is 2 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #225 +/- ## ======================================== Coverage 95.23% 95.23% ======================================== Files 28 28 Lines 1701 1701 ======================================== Hits 1620 1620 Misses 81 81 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

davidorme commented 2 months ago

@AmyOctoCat - the on approval profiling script was broken so it failed its own test when you approved it. Can you reapprove so we can see if that is now fixed.

davidorme commented 2 months ago

@AmyOctoCat I wondered if something like that was needed, or maybe this: https://stackoverflow.com/a/71367604

AmyOctoCat commented 2 months ago

@AmyOctoCat I wondered if something like that was needed, or maybe this: https://stackoverflow.com/a/71367604

Sorry David, only just noticed your comment, that looks like it should do the job. I'm hoping the last commit I pushed achieves something similar, but if it doesn't work, that'll be next port of call...

davidorme commented 2 months ago

~It ran!~ Wait... too early

davidorme commented 2 months ago

That worked! I'm not sure what happens with the actions here - I've seen this before and things just seem to hang for some reason.