Trying to speed up CI a little bit #1131
Conversation
|
Time to beat is 24 mins. |
IAlibay
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1131 +/- ##
==========================================
- Coverage 94.66% 92.52% -2.14%
==========================================
Files 143 143
Lines 11012 11030 +18
==========================================
- Hits 10425 10206 -219
- Misses 587 824 +237
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Now it's roughly 15 mins, so ~ 40% improvement. Still plenty more that could be done, but for the tests I need to run faster this is a win. |
| @pytest.fixture(scope='function') | ||
| # Note: scope on this can sometimes cause segfault, may need to revert to | ||
| # function scope if it happens. | ||
| @pytest.fixture(scope='class') |
There was a problem hiding this comment.
this is purely for speed-up reasons? I'm hesitant about increasing test fixture scope as a shortcut if we're at all concerned about them being mutated.
There was a problem hiding this comment.
(for the future) we discussed this and agreed that we should remove session scopes on everything.
| r.close() | ||
|
|
||
| @pytest.fixture() | ||
| @pytest.fixture(scope='class') |
There was a problem hiding this comment.
(for the future) this one really benefits from a session scope, we'll try to leave it here for now.
| @pytest.fixture(scope='session') | ||
| def charged_benzene(benzene_modifications): | ||
| benzene_offmol = benzene_modifications['benzene'].to_openff() | ||
| benzene_offmol.assign_partial_charges(partial_charge_method='gasteiger') |
There was a problem hiding this comment.
Is it pre-charging the molecule that does the speed-up here? If so, can we just include the charges hard-coded in the file, so that we 1) don't need to generate charges at all and 2) remove any reproducibility issues stemming from the partial charge method?
There was a problem hiding this comment.
We should, but replacing benzene_modifications directly is a bit of a pain because we have tests that check for charges (or lack thereof) that uses these files. So we should do it as a separate data entry (in a separate issue).
There was a problem hiding this comment.
remove any reproducibility issues stemming from the partial charge method?
There are no reproducibility issues with gasteiger charges. They are fully reproducible, all the time.
There was a problem hiding this comment.
According to @jthorton it's not if the hybridization changes - so it's onlys table if rdkit doesn't change the graph
|
let's discuss this at today's meeting |
|
Decision from today's call: @atravitz and @mikemhenry will work on improving dispatched AWS runners and we'll use this to test the workflow where we manually dispatch long jobs & work through a PR. |
|
No API break detected ✅ |
My small laptop can't run tests because they take too long :(
Developers certificate of origin