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

Lint, guard Molecule import #57

Closed
wants to merge 1 commit into from
Closed

Conversation

mattwthompson
Copy link
Member

Fixes #56

Changes made in this Pull Request:

  • Update pre-commit config
  • Re-run linters
  • Avoid Molecule import at high-level import time
>>> import sys
>>> from openff.nagl import __version__
>>> len([key for key in sys.modules if key.startswith("openff.toolkit.utils")])
# upstream/main -> HEAD
12 -> 11
>>> len([key for key in sys.modules if key.startswith("openff.toolkit.topology")])
4 -> 0

Questions

  • Since some of the wrappers here import from various openff.toolkit.utils classes, I'm not sure if all toolkit imports can easily be guarded
  • Import time on my machine is really slow - I haven't profiled this but I'm assuming it's heavy upstreams (PyTorch? DGL?)
  • Open to adding the https://pre-commit.ci/ bot here? Mostly to keep the config up-to-date

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@codecov-commenter
Copy link

Codecov Report

Merging #57 (07fef28) into main (707926f) will increase coverage by 0.00%.
The diff coverage is 86.02%.

Additional details and impacted files

@lilyminium
Copy link
Collaborator

Would it be possible to separate out the linting from the Molecule imports? It's a bit difficult to see what's meaningfully changed 😅

@lilyminium
Copy link
Collaborator

Since some of the wrappers here import from various openff.toolkit.utils classes, I'm not sure if all toolkit imports can easily be guarded

That's a good point. The toolkitwrappers are also somewhat separate from the rest of the code though and should mostly be used in featurising -- I suspect that the vast majority of the imports of openff.nagl.toolkits actually import capture_toolkit_warnings, a non-essential utility that we could potentially upstream (it fixes openforcefield/openff-toolkit#1655).

@lilyminium
Copy link
Collaborator

Import time on my machine is really slow - I haven't profiled this but I'm assuming it's heavy upstreams (PyTorch? DGL?)

Raised #58 to look at!

@lilyminium
Copy link
Collaborator

Open to adding the pre-commit.ci bot here? Mostly to keep the config up-to-date

Sure :)

@mattwthompson
Copy link
Member Author

I think I'll probably two this in two (independent) steps

  1. Lint, possibly with a few smaller PRs into a staging branch so that each can be accepted or declined more easily
  2. Guard toolkit imports, including both Molecule and wrappers (possibly requiring Do not raise warning when allow_undefined_stereo=True openff-toolkit#1705?)

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.

Do not have top-level toolkit imports
3 participants