-
-
Notifications
You must be signed in to change notification settings - Fork 964
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
Add additional docs for importing code with asyncio #2338
base: master
Are you sure you want to change the base?
Conversation
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (3)
Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments
|
||
## Top-level imports | ||
|
||
Suppose your imports happen at the top-level (nearly all code at indentation level 0). Home Assistant will import your code before the event loop starts or import it in the import executor when your integration is loaded. In this case, you likely do not need to consider whether your imports are safe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct term is "module level" since it mentions indentation level 0?
Should we explain there needs to a chain of imports starting from a package's __init__.py
?
|
||
## Imports outside of top-level | ||
|
||
If your imports are not happening at top-level, you must carefully consider each import, as the import machinery has to read the module from disk which does blocking I/O. If possible, it's usually best to change to a top-level import, as it avoids much complexity and the risk of mistakes. Importing modules is both CPU-intensive and involves blocking I/O, so it is crucial to ensure these operations are executed in the executor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are two reasons for doing non module-level imports:
- Breaking circular import chains
- Here, we could maybe mention imports which are only there for the benefit of the type check can be hidden behind an
if TYPE_CHECKING:
guard - We should probably also explain that if the import is imported also via an import chain starting at module-level of
__init__.py
it's safe to do a local import
- Here, we could maybe mention imports which are only there for the benefit of the type check can be hidden behind an
- Avoiding importing a large set of modules which will mostly be unused
- I think this is the case where it's relevant to manually do imports in the executor?
Drafted the PR as changes have been requested. |
Proposed change
Add additional docs for importing code with asyncio
When its safe to import keeps confusing devs so we need to provide more clarity here
Type of change
Additional information