-
Notifications
You must be signed in to change notification settings - Fork 88
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
Only load QC models when running tests #1419
base: main
Are you sure you want to change the base?
Conversation
Looks good. Can we also remove the |
Codecov ReportBase: 99.94% // Head: 99.94% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #1419 +/- ##
=======================================
Coverage 99.94% 99.94%
=======================================
Files 109 109
Lines 3807 3807
Branches 529 529
=======================================
Hits 3805 3805
Misses 1 1
Partials 1 1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
That would have been the simpler solution, but the CLI checks if Line 729 in b1383cc
Lines 837 to 850 in b1383cc
|
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.
Right, so this prevents the run-time evaluation of the QC model but not the importing of the code. So, we couldn't do any extra imports in the qc model (unless we did icky things like put them in the function).
What if we moved the qc package into the tests
directory and imported that at run-time? It could add on the qc_model attribute which we'd use for these tests (or we could come up with a less icky way of doing it later).
We could even just do something like removing the import from stdpopsim/__init__.py
and explicitly import stdpopsim.qc from tests/test_models.py
. I think that would do the trick actually.
Then, we replace the current check to see if there is QC model with a static is_qc_checked
attribute on the class, which we set to True for all existing models and we document the process of setting this to True as part of the QC process?
Yeah, I agree that the only way to avoid importing the qc module is to change the QC process to manually do something to the model as part of the QC (e.g. setting a model flag as you suggest). Maybe we defer this until after the conversion of models to Demes files though? The model implementation and QC process docs will need modifying then anyhow, and we can probably flag models as having been QCed in other ways then (e.g. put non-QCed model files in "staging" folder or something). |
Yeah, sounds good. I just wonder if there's much point in making this change now if it's not really going to affect load time or allow us to include any extra packages for QC. |
Closes #1418