Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Lazy load networkx in topology modules #1874

Merged
merged 2 commits into from
May 29, 2024
Merged

Conversation

mattwthompson
Copy link
Member

I was profiling some other code and found that a ~100 ms import of networkx happens in the toolkit whether or not it's actually used. I remember lazy-loading this in the past, but that's reflected in the current code.

This way of profiling here shows roughly a 50-100 ms speedup depending on how one squints their eyes

$ git checkout upstream/main
$ hyperfine 'python -c "from openff.toolkit import Molecule"' --prepare 'python -c "from openff.toolkit import Molecule"'
Benchmark 1: python -c "from openff.toolkit import Molecule"
  Time (mean ± σ):      1.119 s ±  0.047 s    [User: 0.909 s, System: 0.142 s]
  Range (min … max):    1.085 s …  1.241 s    10 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

$ git checkout lazy-load-networkx
Previous HEAD position was ef2f514f Drop py311 from conda tests (#1873)
Switched to branch 'lazy-load-networkx'
$ hyperfine 'python -c "from openff.toolkit import Molecule"' --prepare 'python -c "from openff.toolkit import Molecule"'
Benchmark 1: python -c "from openff.toolkit import Molecule"
  Time (mean ± σ):      1.050 s ±  0.048 s    [User: 0.864 s, System: 0.121 s]
  Range (min … max):    1.007 s …  1.143 s    10 runs

With this patch, here's a rough picture of what takes so long to run this import. There are some other obvious candidates for lazy-loading, but I'm not going for them now:

image

Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.70%. Comparing base (ef2f514) to head (5e37a6e).
Report is 48 commits behind head on main.

Additional details and impacted files

@mattwthompson mattwthompson marked this pull request as ready for review May 8, 2024 21:30
@mattwthompson mattwthompson changed the title Lazy load networkx in topology molecules Lazy load networkx in topology modules May 8, 2024
Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

Thanks @mattwthompson - Please update the releasenotes before you merge :-)

@mattwthompson mattwthompson merged commit 9ea2783 into main May 29, 2024
16 checks passed
@mattwthompson mattwthompson deleted the lazy-load-networkx branch May 29, 2024 20:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants