Import slayer - #3033
Merged
Merged
Conversation
JonathonMisiewicz
approved these changes
Aug 22, 2023
JonathonMisiewicz
left a comment
Contributor
There was a problem hiding this comment.
Rubber-stamped.
davpoolechem
approved these changes
Aug 23, 2023
This was referenced Nov 13, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
So in the course of working on DDD/pydantic, a fix I needed triggered the dreaded "circular import" error, which anyone who's tried to alter the driver's fragile import structure has probably seen, too. Being sick of this, I located https://medium.com/brexeng/avoiding-circular-imports-in-python-7c35ec8145ed with advice to do
from .driver import energy, notfrom psi4.driver.driver import energy. So that's the first pass on the driver imports. This fixes my circular import problem and makes it easier to find others.Since all the imports are churned anyways, I set up
isort(that is, you can run isort on the repo, not that it's enforced on the repo). This is a utility (https://pycqa.github.io/isort/index.html) that sorts all the imports at the top of the file into stdlib, third_party, and first_party blocks and then alphabetizes the imports within. It also effectively tests the fragility of the import structure by jumbling them all into alphabetical order. Attempts in past years to run isort led to circular imports and a retreat. This time I was able to fix the single one that came about.Third pass is that I ran
autoflakeon the driver to remove unused stdlib imports. It also removed somepasson empty fns that weren't needed because the docstring suffices for the syntax.User notes
import mathbeforemath.pi) that previously were preloaded by psi4.Dev notes & details
molutilintosapt_proc. this is the main one that allows killing off the embarrassingtemp_circular_import_blockerfn. https://github.com/psi4/psi4/compare/master...loriab:psi4:import_slayer?expand=1#diff-f3e93ffa6125dd1f79abad7630bbf259bb1daeee89e983d2889b68af6038875aL35Checklist
tests added for any new featuresStatus