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

Move selfish imports off package __init__.py #14190

Merged
merged 13 commits into from
Jun 21, 2024

Conversation

aaazzam
Copy link
Collaborator

@aaazzam aaazzam commented Jun 20, 2024

Prefect directly / indirectly imports almost every module into it's init.py, which is an obstacle to progress on untangling circular imports or overall import speed.

This PR cheats a little, but in a net-positive way. This moves the contents of the init.py file to a sibling main.py file, and sets up a dynamic importing scheme from to read from it. Here's why this matters:

  1. Before this PR, if I wanted to do something innocent like from prefect.client.orchestration import get_client I would traverse Prefect's init.py which would in turn import every module (by simply visiting it!).
  2. After this PR, since init.py has no declared imports from prefect.client.orchestration import get_client safely traverses down to prefect.client.orchestration and accesses the attribute get_client.

This is no free lunch from the perspective of our users. import prefect is now instant, at the expense that from prefect import flow now incurs the import time instead (importantly though: subsequent from prefect import X do not continue to pay import tax since everything is loaded).

So this PR is purely a path to a path to sanity.

@aaazzam aaazzam marked this pull request as ready for review June 21, 2024 18:56
@aaazzam aaazzam requested review from a team as code owners June 21, 2024 18:56
@aaazzam
Copy link
Collaborator Author

aaazzam commented Jun 21, 2024

Note this can't pass static analysis because our imports are order sensitive and ruff format wants to put them in pep ordering. 🫠

@aaazzam
Copy link
Collaborator Author

aaazzam commented Jun 21, 2024

@chrisguidry I changed the integrations test to use subprocess instead of runpy.run_file. The way that runpy resolves imports I was actually getting an error from this implementation (spooky). I want to make sure I'm not cheating though so your 👀 here are much appreciated.

Copy link
Collaborator

@chrisguidry chrisguidry left a comment

Choose a reason for hiding this comment

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

Looks like a major improvement! We can quibble over the exact public_api but at least now we can make those decisions freely without the whole house of cards collapsing :D

As for the static analysis, I think ruff respects # isort: split, give that a shot. If not we can just ignore this one file.

"__ui_static_path__": __ui_static_path__,
}

_public_api: dict[str, tuple[str, str]] = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙇

@aaazzam aaazzam merged commit 9f20168 into main Jun 21, 2024
25 of 26 checks passed
@aaazzam aaazzam deleted the move-greedy-imports-off-main-package-path branch June 21, 2024 19:22
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.

None yet

2 participants