fix(Python): Support orphaned shapes, update Config shape generation #626

lucasmcdonald3 · 2024-10-08T23:06:37Z

Issue #, if available:

Description of changes:

Python:

Supported "orphaned" shapes (shapes with no graph connection to the LocalService)
Refactor LocalService Config shape generation from its own logic to "normal" shape generation
- Before, Config shapes were "orphaned" and could not be discovered by normal shape generation. I wrote manual generation for Config shapes.
- Now, Config shape generation is more closely tied to normal shape generation, which gets it more fully-featured shape generation (constraints, as/from_dict)
Don't use forward references for boto3 client typehints

Check out this MPL PR for sample generated code: aws/aws-cryptographic-material-providers-library#831

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

lucasmcdonald3 · 2024-10-09T17:27:19Z

...y-python-codegen/src/main/java/software/amazon/smithy/python/codegen/StructureGenerator.java

@@ -252,7 +252,7 @@ protected boolean isOptionalDefault(MemberShape member) {
            && (target.isDocumentShape() || target.isListShape() || target.isMapShape());
    }

-    private void writeClassDocs(boolean isError) {
+    protected void writeClassDocs(boolean isError) {


Unfortunately, this change means another

make mvn_local_deploy_polymorph_python_dependencies

on merging this from main, but I think this is better than duplicating the code.

I'm inclined to add mvn_local_deploy_polymorph_dependencies as a dependency of all polymorph targets, given it should be cheap and hopefully Gradle-cached anyway.

Good idea. I'll include this

robin-aws

Quite happy with the general approach to handle orphaned shapes. Cut #627 for longer term follow up on the problem.

Are you dependent on an OrphanedShapes test model to get added to test these changes?

robin-aws · 2024-10-09T21:25:35Z

...y-python-codegen/src/main/java/software/amazon/smithy/python/codegen/StructureGenerator.java

@@ -252,7 +252,7 @@ protected boolean isOptionalDefault(MemberShape member) {
            && (target.isDocumentShape() || target.isListShape() || target.isMapShape());
    }

-    private void writeClassDocs(boolean isError) {
+    protected void writeClassDocs(boolean isError) {


I'm inclined to add mvn_local_deploy_polymorph_dependencies as a dependency of all polymorph targets, given it should be cheap and hopefully Gradle-cached anyway.

codegen/smithy-dafny-codegen/src/main/java/software/amazon/polymorph/utils/ModelUtils.java

robin-aws · 2024-10-09T21:49:39Z

codegen/smithy-dafny-codegen/src/main/java/software/amazon/polymorph/utils/ModelUtils.java

+        orderedShapes.add(shape);
+      }
+    }
+    for (Shape shape : topologicalIndex.getRecursiveShapes()) {


I wasn't 100% sure from the TopologicalIndex documentation, but is this guaranteed to be a superset of getOrderedShapes()?

Would be nice to have an explicit unit test just for getTopologicallyOrderedOrphanedShapesForService, especially since it can clarify what is or isn't a recursive shape.

I wasn't 100% sure from the TopologicalIndex documentation, but is this guaranteed to be a superset of getOrderedShapes()?

I suspect they're disjoint, otherwise some shapes would be generated multiple times.

Would be nice to have an explicit unit test just for getTopologicallyOrderedOrphanedShapesForService, especially since it can clarify what is or isn't a recursive shape.

I think a unit test might be more effort than it's worth --

Primarily, I'm inclined to trust whatever Smithy-Core is doing here, but verify it against a TestModel/real projects. I built this PR against the MPL and DBESDK, it picked up orphaned shapes, and I can run the code. I don't think the test you're proposing gets us as much ROI.

For a unit test, we'd have to create a Smithy model as a POJO (sort of like this), which seems more brittle than is worth doing.

Let me know your thoughts.

I suspect they're disjoint, otherwise some shapes would be generated multiple times.

Good point, I think they must be.

For a unit test, we'd have to create a Smithy model as a POJO (sort of like this), which seems more brittle than is worth doing.

You can include a *.smithy file as a resource and read it in pretty easily instead: https://github.com/smithy-lang/smithy-dafny/blob/main-1.x/codegen/smithy-dafny-codegen/src/test/java/software/amazon/polymorph/smithyjava/modeled/ModeledShapeValueTest.java#L77-L82

But I'll leave it to your judgement on whether you want to bother with that on this PR, I won't block on it.

That's pretty nifty -- I'll plan to do this when I do the OrphanedShapes TestModel, but not now.

lucasmcdonald3 · 2024-10-09T22:28:32Z

Are you dependent on an OrphanedShapes test model to get added to test these changes?

I don't think so, but let me know if you disagree.

I built this PR into the MPL and DBESDK, and both of those projects are still passing.

MPL PR: fix(Python): Repolymorph to add orphaned shapes aws/aws-cryptographic-material-providers-library#831
DBESDK PR doesn't exist. This project only exists on my local right now.

This (plus the MPL PR) is ideally the last PR I'll to land before I release the Python MPL, so I'm trying to push any non-essential work out until after that lands.
I'm carving out some time to handle these post-launch fast follows, and I added OrphanedShapes TestModel to that list

robin-aws

LGTM, some non-blocking questions

robin-aws · 2024-10-10T21:34:16Z

SmithyDafnyMakefile.mk

@@ -269,6 +272,7 @@ transpile_dependencies_test:
 #   make polymorph_code_gen CODEGEN_CLI_ROOT=/path/to/smithy-dafny/codegen/smithy-dafny-codegen-cli
 # StandardLibrary is filtered out from dependent-model patsubst list;
 #   Its model is contained in $(LIBRARY_ROOT)/model, not $(LIBRARY_ROOT)/../StandardLibrary/Model.
+_polymorph: mvn_local_deploy_polymorph_dependencies


robin-aws · 2024-10-10T21:35:29Z

...olymorph/smithypython/localservice/extensions/DafnyPythonLocalServiceStructureGenerator.java

+      () -> {
+        writer.write("self,");
+        if (!shape.members().isEmpty()) {
+          // Adding this star to the front prevents the use of positional arguments.


Was this lifted from smithy-python too then? It's a great idea but I wasn't sure if they'd acted on it yet.

robin-aws · 2024-10-10T21:36:29Z

...olymorph/smithypython/localservice/extensions/DafnyPythonLocalServiceStructureGenerator.java

@@ -203,7 +309,14 @@ protected void writePropertyForMember(
      );
    }

-    if (target.hasTrait(ReferenceTrait.class)) {
+    // Reference shapes require forward reference to avoid circular import,
+    // but references to AWS SDKs don't


Nit picking, but isn't it more accurately that shapes from dependencies are already declared?

This is actually about circular dependencies within one project.
I'm going to write about this problem and solutions in a Smithy-Dafny Python dev guide, but an overview/instance of the problem is that models.py can't import references.py at the top-level because references.py depends on models.py at the top-level, but models.py still needs to import references.py to use its classes.
One solution is to defer the imports like I've done here.
I wrote a fair amount about this and drew some graphs, but I'm going to collate all that later.

I will be thrilled to review a PR that targets docs/python in the future :)

robin-aws · 2024-10-10T21:38:49Z

...python/wrappedlocalservice/extensions/DafnyPythonWrappedLocalServiceClientCodegenPlugin.java

+  ) {
+    Model transformedModel = model;
+    transformedModel =
+      addWrappedLocalServiceTrait(transformedModel, serviceShape);


off topic question: what does this transformation do exactly?

Check out https://github.com/smithy-lang/smithy-dafny/blob/main-1.x/codegen/smithy-dafny-codegen/src/main/java/software/amazon/polymorph/smithypython/wrappedlocalservice/extensions/DafnyPythonWrappedLocalServiceClientCodegenPlugin.java#L52

In preprocessing, it replaces a LocalServiceTrait on the ServiceTrait with a WrappedLocalServiceTrait.
This results in some different codegen; wrapped LocalServices don't get all the codegen from LocalServices. (No models, etc.)

Ah thanks! I wasn't aware of that trait.

…626)

lucasmcdonald3 added 30 commits October 7, 2024 15:48

m

4e7240e

m

bf0df25

m

689cee0

m

35427ab

m

04bd5ec

m

495ee9f

m

9fc74b6

m

a8a4555

m

4c100b5

m

523e3c3

m

0d00cd9

m

6a65d57

m

f1c3fca

m

cfe7a3c

m

3ce6972

m

21293bf

m

66f4aa3

m

ec48abf

m

d07fcf8

m

a66909e

m

e2e5205

m

18c56cf

m

0034c85

m

237feb2

m

38ff2ef

m

7602df4

m

6d2c296

m

26b0be1

m

b56c840

m

1e83075

Merge branch 'main-1.x' into config-unions

fbbd736

lucasmcdonald3 marked this pull request as ready for review October 9, 2024 00:03

lucasmcdonald3 requested a review from a team as a code owner October 9, 2024 00:03

m

d97fdd8

lucasmcdonald3 changed the title ~~fix(Python): Support orphaned shapes, update type generation~~ fix(Python): Support orphaned shapes, update Config shape generation Oct 9, 2024

m

c8129ea

lucasmcdonald3 commented Oct 9, 2024

View reviewed changes

lucasmcdonald3 added 12 commits October 9, 2024 10:54

m

02c7c72

m

6d4d90b

m

d62d4b2

m

f1944b7

m

435f6bc

m

aaf7703

m

aa3f39c

m

aca1d18

m

8cc1633

m

a79d973

m

288d6ff

Merge branch 'main-1.x' into config-unions

58056e9

robin-aws requested changes Oct 9, 2024

View reviewed changes

lucasmcdonald3 added 5 commits October 9, 2024 15:45

m

d3dc8b1

m

b288469

Merge branch 'main-1.x' into config-unions

f5937e0

Merge branch 'main-1.x' into config-unions

4f7a90b

Merge branch 'main-1.x' into config-unions

3dd18c6

robin-aws approved these changes Oct 10, 2024

View reviewed changes

lucasmcdonald3 merged commit 0ac4a65 into main-1.x Oct 10, 2024
80 checks passed

lucasmcdonald3 deleted the config-unions branch October 10, 2024 21:59

ShubhamChaturvedi7 pushed a commit that referenced this pull request Oct 18, 2024

fix(Python): Support orphaned shapes, update Config shape generation (#…

1005afa

…626)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Python): Support orphaned shapes, update Config shape generation #626

fix(Python): Support orphaned shapes, update Config shape generation #626

lucasmcdonald3 commented Oct 8, 2024 •

edited

Loading

lucasmcdonald3 Oct 9, 2024

robin-aws Oct 9, 2024

lucasmcdonald3 Oct 9, 2024

robin-aws left a comment

robin-aws Oct 9, 2024

robin-aws Oct 9, 2024

lucasmcdonald3 Oct 9, 2024 •

edited

Loading

robin-aws Oct 9, 2024

lucasmcdonald3 Oct 9, 2024

lucasmcdonald3 commented Oct 9, 2024

robin-aws left a comment

robin-aws Oct 10, 2024

robin-aws Oct 10, 2024

lucasmcdonald3 Oct 10, 2024

robin-aws Oct 10, 2024

lucasmcdonald3 Oct 10, 2024

robin-aws Oct 10, 2024

robin-aws Oct 10, 2024

lucasmcdonald3 Oct 10, 2024 •

edited

Loading

robin-aws Oct 10, 2024

fix(Python): Support orphaned shapes, update Config shape generation #626

fix(Python): Support orphaned shapes, update Config shape generation #626

Conversation

lucasmcdonald3 commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robin-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmcdonald3 Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmcdonald3 commented Oct 9, 2024

robin-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmcdonald3 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmcdonald3 commented Oct 8, 2024 •

edited

Loading

lucasmcdonald3 Oct 9, 2024 •

edited

Loading

lucasmcdonald3 Oct 10, 2024 •

edited

Loading