Skip to content
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

Faster, more space-efficient tutorials #1124

Merged
merged 43 commits into from
Mar 29, 2023
Merged

Faster, more space-efficient tutorials #1124

merged 43 commits into from
Mar 29, 2023

Conversation

adamjstewart
Copy link
Collaborator

@adamjstewart adamjstewart commented Feb 18, 2023

This PR attempts to simplify and speed up our notebook tests, and includes the following changes:

  • Use nbmake variable mocking instead of checking pytest env vars
  • Use tempfile for a single shared download directory
  • Download smaller datasets
  • Strip output from notebooks to make smaller files
  • Rerun tests if they fail, known transient issues

Closes #665
Closes #1074

Before

Numbers from here.

Tutorial Size Time
benchmarking 1.9G (NAIP) + 717M (Chesapeake) 34m 26s
custom dataset 331M (Sentinel) 4m 54s
getting started 1.9G (NAIP) + 717M (Chesapeake) 10s
indices 331M (Sentinel) 59s*
pretrained weights 4.7G (EuroSAT) 1m 25s
trainers 6.4G (Cyclone) 5m 28s*
transforms 4.7G (EuroSAT) 2m 37s
total 22G† 50m*

*failed, likely longer if passed
†duplicate copies of each download

After

Numbers from here.

Tutorial Size Time
benchmarking 1.9G (NAIP) + 717M (Chesapeake) 2m 1s
custom dataset 331M (Sentinel) 3m 11s
getting started 1.9G (NAIP) + 717M (Chesapeake) 10s
indices 19M (EuroSAT100) 10s
pretrained weights 19M (EuroSAT100) 15s
trainers 19M (EuroSAT100) 13s
transforms 19M (EuroSAT100) 1m 23s
total 3.0G†† 7m 34s

††shared copies across downloads

@adamjstewart adamjstewart added this to the 0.4.1 milestone Feb 18, 2023
@github-actions github-actions bot added datasets Geospatial or benchmark datasets dependencies Packaging and dependencies documentation Improvements or additions to documentation testing Continuous integration testing and removed datasets Geospatial or benchmark datasets labels Feb 18, 2023
@adamjstewart adamjstewart removed the dependencies Packaging and dependencies label Feb 20, 2023
@calebrob6
Copy link
Member

This is waiting on the 100 image version of EuroSat right?

@adamjstewart
Copy link
Collaborator Author

Yep, will start trying to integrate that now.

@github-actions github-actions bot added the dependencies Packaging and dependencies label Feb 24, 2023
@adamjstewart adamjstewart changed the title Speed up notebook tests Faster, more space-efficient tutorials Feb 24, 2023
@adamjstewart
Copy link
Collaborator Author

Notebook tests are passing for the first time in almost a year!

@adamjstewart
Copy link
Collaborator Author

I think this PR is mostly complete. I couldn't get caching working, but we can figure that out another day. A few things remaining I'm concerned about:

  • nbmake seems to still be finicky, it crashes if I save the cell output in the trainers tutorial
  • reviewing git diffs is impossible, wish there was a way to improve this

Depending on whether the notebook was saved locally or on Colab, the indentation changes, meaning every single line is changed (maybe we can find a JSON autoformatter to fix this?). And saving on Colab adds a ton of extraneous metadata that I don't want. We could remove all outputs, which would fix nbmake and reduce the size of the files, but then you wouldn't be able to see plots without running tutorials. We could also use nbsphinx to generate the outputs, but this would be slow and require downloads and need to happen on every commit. We would at least want to make smaller downloads for the last 3 tutorials.

@adamjstewart
Copy link
Collaborator Author

Would also be nice if we could run isort/flake8/pyupgrade on our notebooks. Or even store the notebook in a .py file and autoconvert it to .ipynb on the fly like PyTorch does. But I know less about those.

@adamjstewart
Copy link
Collaborator Author

Opened a discussion for this: #1152

I guess I'm fine merging this PR as is, although it would be nice to decide whether we want to include outputs in tutorials before we merge this PR which adds 10K lines of code.

@adamjstewart adamjstewart marked this pull request as ready for review March 1, 2023 20:07
isaaccorley
isaaccorley previously approved these changes Mar 10, 2023
@adamjstewart
Copy link
Collaborator Author

Not sure why the tests are crashing again. They were working fine on Colab. May have to try stripping outputs again.

@adamjstewart adamjstewart marked this pull request as draft March 18, 2023 16:49
@adamjstewart adamjstewart marked this pull request as ready for review March 20, 2023 19:23
env:
MLHUB_API_KEY: ${{ secrets.MLHUB_API_KEY }}
run: pytest --nbmake --nbmake-timeout=3000 docs/tutorials --durations=10
run: pytest --nbmake --durations=10 --reruns=10 docs/tutorials
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--durations=10 prints the 10 slowest tests (very useful for seeing which tests are worth speeding up). --reruns=10 reruns the tests up to 10 times until they pass.

Even with all the changes in this PR, the tests seem to still fail intermittently. I'm hoping that once treebeardtech/nbmake#80 is solved, the error message will be more useful. Until then, rerunning the tests is necessary to ensure that they pass.

@adamjstewart adamjstewart merged commit a0455d4 into main Mar 29, 2023
@adamjstewart adamjstewart deleted the tests/notebooks branch March 29, 2023 01:26
calebrob6 pushed a commit that referenced this pull request Apr 10, 2023
* Speed up notebook tests

* Black fix

* Mock rest of variables

* Undo URL changes

* Update conda deps

* Notebooks also plot images

* Fix undefined variable

* Test with serial data loading

* Use tempfile for all data download directories

* Encode timeout in notebook

* Share datasets across processes

* Fix missing import

* Pretrained Weights: use EuroSAT100

* Transforms: use EuroSAT100

* Trainers: use EuroSAT100

* Blacken

* MPLBACKEND is already Agg by default on Linux

* Indices: use EuroSAT100

* Pretrained Weights: add output

* Pretrained Weights: add output

* Trainers: save output

* Pretrained Weights: ResNet 50 -> 18

* Trainers: better graph

* Indices: add missing plot

* Cache downloads

* Small edit

* Revert "Cache downloads"

This reverts commit 5276c53.

* Revert "Revert "Cache downloads""

This reverts commit 137c69e.

* env only

* half env

* Variable with no braces

* Set tmpdir elsewhere

* Give up on tmpdir caching

* Trainers: clear output

* lightning.pytorch package import

* nbstripout

* Rerun upon failure

* Re-add caching

* Rerun failures on release branch too
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Speed up notebook tests

* Black fix

* Mock rest of variables

* Undo URL changes

* Update conda deps

* Notebooks also plot images

* Fix undefined variable

* Test with serial data loading

* Use tempfile for all data download directories

* Encode timeout in notebook

* Share datasets across processes

* Fix missing import

* Pretrained Weights: use EuroSAT100

* Transforms: use EuroSAT100

* Trainers: use EuroSAT100

* Blacken

* MPLBACKEND is already Agg by default on Linux

* Indices: use EuroSAT100

* Pretrained Weights: add output

* Pretrained Weights: add output

* Trainers: save output

* Pretrained Weights: ResNet 50 -> 18

* Trainers: better graph

* Indices: add missing plot

* Cache downloads

* Small edit

* Revert "Cache downloads"

This reverts commit 5276c53.

* Revert "Revert "Cache downloads""

This reverts commit 137c69e.

* env only

* half env

* Variable with no braces

* Set tmpdir elsewhere

* Give up on tmpdir caching

* Trainers: clear output

* lightning.pytorch package import

* nbstripout

* Rerun upon failure

* Re-add caching

* Rerun failures on release branch too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Packaging and dependencies documentation Improvements or additions to documentation testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trainers tutorial: switch dataset Faster notebook testing
3 participants