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

Importing everything in utils module #698

Merged
merged 6 commits into from
Jun 5, 2023
Merged

Conversation

ijpulidos
Copy link
Contributor

Description

Importing all members of the utils.py submodule.

In previous changes we created a openmm.utils package/module with a utils.pysub module for the sake of organizing and modularizing many of the utility functions we have in openmmtools. By doing so I mistakenly only imported some of the members of the modules instead of importing all of them.

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

  • Ready to go

Changelog message

We are now importing all the members of the new `utils.py` submodule by default. Recovers previous behavior of importing utils with `from openmm.utils import ...`, avoiding API breaking changes.

@ijpulidos
Copy link
Contributor Author

This should fix the problems importing the openmmtools utils in downstream packages, such as in choderalab/yank#1286 and choderalab/yank#1299

typename,
with_timer,
)
from .utils import *
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to import everything into the namespace?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't skimmed through to make sure that we don't have collisions, if this is exactly how it was working, then that is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, there's no easy way to know what packages downstream are using, my first attempt was trying NOT to import everything, and only the things that openmmtools itself was needing, but it's hard to tell what other packages are using from utils.py so the easiest way is to import everything. I think in the future we probably want to remove the utils.py file and just have submodules that make more sense (such as what we did with the gentle equilibration utility function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't skimmed through to make sure that we don't have collisions, if this is exactly how it was working, then that is fine.

Yeah, it should be analogous to what was happening before the changes, such that users can still do import openmmtools.utils (or similar)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay then I think it is okay! If we ever do a big breaking change, we can cleanup the API but for now lets make downstream life easy

@mikemhenry
Copy link
Contributor

@ijpulidos looks like the tests need fixing (or change the import to be backwards compatible)

@ijpulidos
Copy link
Contributor Author

@ijpulidos looks like the tests need fixing (or change the import to be backwards compatible)

I think you were right on doubting using the * from the start :). I think we just need to be explicit on the imports. I don't know about the details, but apparently private attributes/variables/members (those that start with an underscore for example) are not imported when we do from module import * .

@ijpulidos ijpulidos requested a review from mikemhenry May 31, 2023 00:56
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #698 (b328194) into main (c491c3d) will not change coverage.
The diff coverage is n/a.

@mikemhenry mikemhenry enabled auto-merge (squash) May 31, 2023 16:48
Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

Can you run a sort on these? Or can I?

Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

LGTM, I ran a sort on the imports

@mikemhenry mikemhenry disabled auto-merge June 5, 2023 23:49
@mikemhenry mikemhenry merged commit c130f98 into main Jun 5, 2023
@mikemhenry mikemhenry deleted the fix-include-all-utils branch June 5, 2023 23:49
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.

2 participants