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

feat: remove root-level import of all modules in python #3030

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Jul 24, 2023

In python importing a submodule leads to all __init__.py files below the import path being executed. We have an index.ts with a namespaced import for all submodules as an entry point for JSII (It needs to be there so all the submodules are being registered). This leads to a from a provider point of view top-level __init__.py importing all the namespaces, so even if users use specific submodule imports they import the entire provider.

This change cuts the synth time in half for the AWS example (from 30s to 15s).

This is a breaking change since top-level imports (from imports.aws import s3Bucket) don't work anymore, they need to be replaced with namespace imports (from imports.aws.s3_bucket import S3Bucket).

Additionally this PR upgrades JSII and JSII-Srcmak, allowing us to use the new lazy loading mechanism within JSII bringing the initial synth time down from 15s to 5s and every iterative synth (after the providers and modules are in the JSII cache) down to 2s.

TL;DR: Breaking change bringing Pythons synth time down from 30s to 2s

@DanielMSchmidt DanielMSchmidt requested a review from a team as a code owner July 24, 2023 10:44
@DanielMSchmidt DanielMSchmidt requested review from mutahhir and ansgarm and removed request for a team July 24, 2023 10:44
@DanielMSchmidt DanielMSchmidt force-pushed the improve-python-performance-by-removing-imports-from-root-level-init branch from fe27fd3 to dd99965 Compare August 2, 2023 11:48
@DanielMSchmidt DanielMSchmidt force-pushed the improve-python-performance-by-removing-imports-from-root-level-init branch from 554991c to 3f9b9dd Compare August 2, 2023 12:04
@DanielMSchmidt DanielMSchmidt force-pushed the improve-python-performance-by-removing-imports-from-root-level-init branch from 3f9b9dd to 5f8e677 Compare August 2, 2023 12:31
Copy link
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

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

Looking good! Only a small typo and a question.

As this is a breaking change, I think we should add an unreleased section to the changelog and give some before & after examples for this change. Upon the next release, we should also copy that to an upgrade guide (not sure if adding an upgrade guide already would release it too soon 😄)

@DanielMSchmidt DanielMSchmidt force-pushed the improve-python-performance-by-removing-imports-from-root-level-init branch from 133819c to 60f51d1 Compare August 10, 2023 17:01
@DanielMSchmidt DanielMSchmidt merged commit b4b766e into main Aug 11, 2023
41 of 43 checks passed
@DanielMSchmidt DanielMSchmidt deleted the improve-python-performance-by-removing-imports-from-root-level-init branch August 11, 2023 12:31
@mutahhir mutahhir mentioned this pull request Aug 17, 2023
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm provider: Python package cdktf-cdktf-provider-azurerm extremely slow to import
3 participants