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

fix(python): unknown type when submodule is not loaded #3049

Merged
merged 8 commits into from
Oct 11, 2021

Conversation

RomainMuller
Copy link
Contributor

Callbacks may make references to types from submodules of loaded
packages, however due to how the type registration works in the Python
runtime, if the submodule has not been loaded, then the types it
contains are not registered and will fail resolving.

The simplest possible fix is to preemtively load all submodules upfront
at the end of the root module's __init__.py file, which guarantees
that all types have correctly been registered in the runtime (at the
expense of some time and memory).

Fixes aws/aws-cdk#16625


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Callbacks may make references to types from submodules of loaded
packages, however due to how the type registration works in the Python
runtime, if the submodule has not been loaded, then the types it
contains are not registered and will fail resolving.

The simplest possible fix is to preemtively load all submodules upfront
at the end of the root module's `__init__.py` file, which guarantees
that all types have correctly been registered in the runtime (at the
expense of some time and memory).

Fixes aws/aws-cdk#16625
@RomainMuller RomainMuller requested a review from rix0rrr October 8, 2021 14:19
@RomainMuller RomainMuller self-assigned this Oct 8, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 8, 2021
@RomainMuller RomainMuller added the pr/ready-to-merge This PR is ready to be merged. label Oct 11, 2021
@RomainMuller
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 11, 2021

Command refresh: success

Pull request refreshed

@RomainMuller RomainMuller merged commit da55a1e into main Oct 11, 2021
@RomainMuller RomainMuller deleted the rmuller/python-submodules branch October 11, 2021 13:18
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Oct 11, 2021
@jsteinich jsteinich mentioned this pull request Oct 19, 2021
5 tasks
mergify bot pushed a commit that referenced this pull request Nov 18, 2021
The goal of this PR is, to find a way for generated Python code to work with namespaced modules regardless of the way it is consumed. While the current implementation of submodule imports seem to work fine when distributed as packages, it's breaking apart when the generated code is imported relatively as part of a single code base. That's what we do in cdktf and the reason why we have locked jsii to `1.37.0` at the moment.

The general idea is, to generate relative imports rather than fqn imports in Python. This could be a potential way to address #3078

```python
# Before    
import jsii_calc.submodule
import jsii_calc.submodule.nested_submodule
import jsii_calc.submodule.nested_submodule.deeply_nested

# After
from . import submodule
from .submodule import nested_submodule
from .submodule.nested_submodule import deeply_nested
```

Besides from a few snapshots being changed, it seems like the tests are still working with this change. However, I'm not sure  about the general implications. So, this entire PR is really meant to be a conversation starter. 

I have confirmed that an import like `import . from some_aws_namespace` would work for us in cdktf.

## References

- #3078
- #3049
- hashicorp/terraform-cdk#1152
- hashicorp/terraform-cdk#1288

---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(jsii)(v2): Cannot apply Aspects to EKS cluster stack
2 participants