-
Notifications
You must be signed in to change notification settings - Fork 12
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
adding notebook tests #308
Conversation
@rfl-urbaniak , now that you've merged in #318 would you like @eb8680 to review this PR or are you planning on adding more notebooks first? |
I've added backdoor.ipynb and will keep adding notebooks tomorrow. Will ask for review and change status when done. |
Includes notebook tests for Two excluded notebooks are
If no revisions are suggested, this PR can be merged, and we can deal with these two remaining notebooks later. |
@eb8680 this can be reviewed now (see a comment above, though). |
@rfl-urbaniak , it looks like the CEVAE notebook has an error in it. Could you please rerun and confirm that it works end-to-end. |
@SamWitty So the notebooks pass the automated tests on a GitHub runner. They pass locally on my laptop. They fail locally on my desktop. torchaudio 0.13.1+cpu has requirement torch==1.13.1, but you have torch 2.1.0. The problem is, as soon as I downgrade torch to meet this requirement, pyro ppl compains about requiring torch > 2.0. |
@rfl-urbaniak , does this conflict persist with a fresh start from a clean virtual environment? If so, I'd recommend removing the CEVAE notebook and submitting this PR for review without it. We can add back more notebooks in subsequent PRs. |
@rfl-urbaniak , is this ready for review? You added the tag |
After changing the status, I re-run the notebooks locally outside of the CI=1 context to recover the original figures and pushed. Should be ready for review. |
Great. @eb8680 , I was fairly involved in setting this up, so I think it makes sense for you to review and merge (if it passes your review). I'll remove myself from the reviewers on GH. |
…into ru-notebook-testing
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.
Thanks for tackling this, seems like a reasonable starting point. If it ends up adding too much time to CI builds, we can try to parallelize over the notebooks at the level of the GitHub workflow, rather than within a single CI instance via pytest-xdist
.
It would also be nice to typecheck and lint notebooks in CI, if possible. I will create a separate issue for that.
Partially addresses an existing issue #283 (which shouldn't be resolved automatically upon merging this PR).
For now with
tutorial_i
, as smoke screen conditioning needs to be added to other notebooks to speed them up. I can keep going with the other notebooks, but for now, it seemed sensible to touch base and agree on the method.Sam was fantastic at teaching me to include smoke tests, helping me convert my Python script to a shell script, and commenting on an earlier version of my workflow file. Thanks @SamWitty! After our conversation, the following transpired:
seaborn
ormatplotlib
). I added aseparate notebook requirements.txt
indocs/source
, and referred to it and torequirements.txt
intest_notebooks.yml
. The workflow also needed to usets-graphviz/setup-graphviz@v1
for the notebooks to be able to produce our precious model graphs.