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

Decrease run time #21

Merged
merged 13 commits into from
Feb 25, 2021
Merged

Decrease run time #21

merged 13 commits into from
Feb 25, 2021

Conversation

joaander
Copy link
Member

Description

Decrease the run time of the examples.

  • Decrease system size
  • Decrease run length accordingly
  • Reduce fresnel image resolution and number of samples

On my linux box, they now run in 10 minutes. On Azure, they run in 20 minutes (compare to 5 hours previously).

Also:

  • Run all examples on the CPU
  • Adjust outputs to remove pointer addresses

With these changes, the notebook outputs are now as reproducible as they can be. The HOOMD trajectories and the fresnel renderings do not change when rerun on the same system. We may make use of this in the future and have the CI committing the notebook output.

The only changes in the diffs on rerunning (with the same binaries) are now:

  • walltime/TPS output in the logging tutorial
  • iopub exeuction timing in all cells

There is supposed to be a record_timing metadata key and/or nbconvert option to prevent iopub date/time information from changing the cell metadata every time the notebook is rerun. However, I was unable to get this option to have any effect.

Checklist:

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@joaander joaander requested a review from b-butler February 17, 2021 13:44
@bdice
Copy link
Member

bdice commented Feb 17, 2021

@joaander You might find this set of pre-commit hooks helpful as well:
https://github.com/glotzerlab/signac-examples/blob/a3d1729ea716cec2f8e51e950fcb6e0f14ef2775/.pre-commit-config.yaml

  • strips metadata (iopub and more)
  • replaces absolute paths with repository-relative paths in the output cells (/home/joaander/.../project/notebooks/Test.ipynb becomes notebooks/Test.ipynb)
  • Formats input cells with black/isort/pyupgrade

@joaander
Copy link
Member Author

@joaander You might find this set of pre-commit hooks helpful as well:
https://github.com/glotzerlab/signac-examples/blob/a3d1729ea716cec2f8e51e950fcb6e0f14ef2775/.pre-commit-config.yaml

  • strips metadata (iopub and more)
  • replaces absolute paths with repository-relative paths in the output cells (/home/joaander/.../project/notebooks/Test.ipynb becomes notebooks/Test.ipynb)
  • Formats input cells with black/isort/pyupgrade

If you think it would be useful, feel free to make a PR. We do need to preserve some metadata (nbsphinx hidden cells and a tag that allows the notebook to continue when a cell throws an excption).

@joaander joaander enabled auto-merge (squash) February 18, 2021 11:48
@joaander joaander requested review from a team and tommy-waltmann and removed request for a team February 25, 2021 12:11
@joaander joaander disabled auto-merge February 25, 2021 12:13
@review-notebook-app
Copy link

review-notebook-app bot commented Feb 25, 2021

View / edit / reply to this conversation on ReviewNB

tommy-waltmann commented on 2021-02-25T13:30:40Z
----------------------------------------------------------------

Let's make the variable on the first line be cpu instead of gpu 


joaander commented on 2021-02-25T13:45:28Z
----------------------------------------------------------------

Good catch.

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 25, 2021

View / edit / reply to this conversation on ReviewNB

tommy-waltmann commented on 2021-02-25T13:30:41Z
----------------------------------------------------------------

Same as above comment here


@tommy-waltmann
Copy link

Does the current runtime of the notebooks ensure they aren't a bottleneck for CI now?

Copy link
Member Author

Good catch.


View entire conversation on ReviewNB

@joaander
Copy link
Member Author

Does the current runtime of the notebooks ensure they aren't a bottleneck for CI now?

Yes, the runtime reduction from 5 hours to 20 minutes is significant.

Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

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

The changes LGTM. Also the cut down in time is great!

@bdice
Copy link
Member

bdice commented Feb 25, 2021

I’ll pause on #22 until this is merged, to avoid burning tons of CI time.

@joaander joaander merged commit 1b87e27 into v3-api Feb 25, 2021
@joaander joaander deleted the shorten-run-time branch February 25, 2021 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants