-
Notifications
You must be signed in to change notification settings - Fork 172
speed up tests #2189
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
base: main
Are you sure you want to change the base?
speed up tests #2189
Conversation
jamesmkrieger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable so thanks for this fix. Please revert other changes where doc strings and assert messages have been deleted though to make as clean a change as possible
also, it seems like some of the checks don’t start anymore, so please double check that nothing got broken. It seems to be sporadic though
|
Thanks @jamesmkrieger. I will return the doc strings that AI removed. The checks were sporadically not starting before these changes (I saw yesterday from Isabelle's PR on the hinge finding code). I will look into that and fix. I also found the other tests taking too long and will fix those as well in a separate PR. |
|
Hi @jamesmkrieger, I re-generated the "fixed" test with doc strings and keeping all original assert statements. I also modified main.yml to hopefully fix the intermittent CI testing failures. |
|
Now it only takes 30 mins! And hopefully the intermittent crashes are fixed... |
|
Now it's down to 15ish minutes. |
|
OK it's under 10 mins now (Ubuntu ones are faster than the macos ones, but not a big deal). That might be the limit. The tests I changes were pfam, bioexcel, anmd, insty, prody_catdcd, prody_examples and prody_pca. Also, the main.yml was modified for faster env setup and stability. @jamesmkrieger I think I am done with this. I made sure original doc strings, formatting, comments and assert statements were preserved in each test. |
|
That’s wonderful! I’ll have a look over the next few days |
jamesmkrieger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. Please do make sure that there is still one query each to pfam and bioexcel to make sure we’re keeping in line with changes there
Great, thanks for the review! I'll work on addressing these tomorrow. |
I (with help from Gemini) re-wrote the pfam tests to not query the server each time. I think this should significantly speed up the tests through GitHub CI. I narrowed it down to this test by looking at time per tests and some of these would randomly hang for like 10-20 mins. There could be other culprits, but maybe this is a good place to start.
Also includes fixes for intermittent test failure and speeds up other tests like test_bioexcel.py.