[codegen] Output-versioned function invokes #612

Zaid-Ajaj · 2022-05-23T14:35:17Z

Description

This PR is my initial attempt at getting function invokes to return Output<ResultType> instead of CompletableFuture<ResultType>

My thought at first was to generate syntax sugar around existing function invokes that return CompletableFuture<ResultType> and wrap them using Output.all, Output.apply and Output.of(CompletableFuture) but then I saw that the SDK itself has the function that can be used immediately:

Deployment.getInstance().invoke(....) // returns Output<T>

Questions are

Should both types of function invokes co-exist? if we have functions that return Output<T> is there any need for CompletableFuture alternative?
Am I on the right track with this implementation?

TODO

Generate code from all providers
Remove workaround in programgen that wraps function invokes with Output.of(....)

Checklist

I have added tests that prove my fix is effective or that my feature works

I have updated the CHANGELOG-PENDING file with my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

pawelprazak

Thank you, looks promising.

btw. IMHO this change shows architectural issues already in code that should be addressed as a separate issue.

pkg/codegen/java/gen.go

pawelprazak · 2022-05-23T15:07:25Z

Should both types of function invokes co-exist? if we have functions that return Output is there any need for CompletableFuture alternative?

this is a very good question, I second this

pawelprazak · 2022-05-23T15:33:26Z

providers/pulumi-aws/sdk/java/src/main/java/com/pulumi/aws/acm/AcmFunctions.java

-    public static CompletableFuture<GetCertificateResult> getCertificate(GetCertificateArgs args, InvokeOptions options) {
-        return Deployment.getInstance().invokeAsync("aws:acm/getCertificate:getCertificate", TypeShape.of(GetCertificateResult.class), args, Utilities.withVersion(options));
+    public static Output<GetCertificateResult> getCertificate(GetCertificateArgs args, InvokeOptions options) {
+        return Deployment.getInstance().invoke("aws:acm/getCertificate:getCertificate", TypeShape.of(GetCertificateResult.class), args, Utilities.withVersion(options));


yes, good find! I've added this method for this purpose

pawelprazak · 2022-05-25T09:09:13Z

A kind reminder, to clear the codegen test errors we currently need to manually commit the changes to the generated test code.

Running PULUMI_ACCEPT=true; make codegen_tests and committing the result should clear the current CI error IIRC.

Sorry for the inconvenience, we probably should automate this somehow in the future.

Zaid-Ajaj · 2022-05-25T09:39:43Z

Hi @pawelprazak, been trying to debug what is going on here to no avail 😓 I ran the command you sent but the tests keep failing. It is showing code diffs telling that the generated code has changed ... well, that's what the PR is supposed to do 😅 any leads?

pawelprazak · 2022-05-26T08:42:23Z

@Zaid-Ajaj I'm sorry you're having troubles with this, if you'd like we could pair on this over zoom.
Feel free to reach out to me on Slack.

Zaid-Ajaj · 2022-05-26T13:15:37Z

@pawelprazak thank you! Tomorrow I will have another look and reach out if I am still stuck 🙏

Zaid-Ajaj · 2022-05-29T19:08:31Z

NOTE to self:

Remember to PUMULI_ACCEPT=true make codegen_tests
Having an outdated build of the SDK in your local maven build will cause the tests to fail unless you make install_sdk first before running the tests

Zaid-Ajaj · 2022-05-29T22:01:46Z

Alright so this is really frustrating to work with, after having changed how function invokes are implemented, regenerated code from providers and installed them locally, the test examples failed in the CI. For example

make test_example.gcp-java-gke-hello-world

would fail in the CI but if I run it locally it tells me that everything is good while using a older version of the provider (gcp in this case)

This means I was in the unfortunate state of ✅ locally and 🔴 on CI and had to rewrite the function invokes in the test examples by hand to make them work in CI but locally it was red squiggles all over the place.

Somehow even when I install a provider after regenerating it, the test examples were still using a cached build somewhere on my machine. I would expect that make test_example.gcp-java-gke-hello-world or make provider.gcp.install would take care of removing cached builds of providers.

The test examples are now all green which is good but still need to update azure-java template. Specifically, the following

    private static Output<String> getStorageAccountPrimaryKey(Output<String> resourceGroupName,
                                                              Output<String> accountName) {
        return Output.tuple(resourceGroupName, accountName).apply(tuple -> {
            var actualResourceGroupName = tuple.t1;
            var actualAccountName = tuple.t2;
            var invokeResult = StorageFunctions.listStorageAccountKeys(ListStorageAccountKeysArgs.builder()
                    .resourceGroupName(actualResourceGroupName)
                    .accountName(actualAccountName)
                    .build());
            return Output.of(invokeResult)
                    .applyValue(r -> r.keys().get(0).value())
                    .asSecret();
        });
    }

should change into:

    private static Output<String> getStorageAccountPrimaryKey(Output<String> resourceGroupName,
                                                              Output<String> accountName) {
        return Output.tuple(resourceGroupName, accountName).apply(tuple -> {
            var actualResourceGroupName = tuple.t1;
            var actualAccountName = tuple.t2;
            var invokeResult = StorageFunctions.listStorageAccountKeys(ListStorageAccountKeysArgs.builder()
                    .resourceGroupName(actualResourceGroupName)
                    .accountName(actualAccountName)
                    .build());
            return invokeResult
                    .applyValue(result -> result.keys().get(0).value())
                    .asSecret();
        });
    }

I guess I should update that in the templates repo and then update the templates submodule from here

Zaid-Ajaj · 2022-05-29T22:07:00Z

NOTE: function invokes here also suffer from pulumi/pulumi#9593 which is why wrapping with Output.tuple(...).apply(...) is still necessary sometimes when doing a pulumi preview

Zaid-Ajaj · 2022-05-30T11:41:38Z

TODOs

Generate code for all providers
Refactor program-gen to use output-versioned invokes
Disable testing templates because these are out of date with latest providers
Bring back function invokes that return CompletableFuture<T> as there is no (easy) way to obtain T from Output<T> and use it in control flow unless from within apply and applyValue (thanks @mikhailshilkov for the note)

…s tests

t0yv0 · 2022-05-31T15:50:27Z

Should both types of function invokes co-exist? if we have functions that return Output is there any need for CompletableFuture alternative?

Both must coexist, unless we opt to expose Output.join() : Output<T> -> T. There are scenarios where invokes need to run in plain context and lifting them to work on Output modality is too much pain on the user.

t0yv0 · 2022-05-31T16:00:44Z

My thought at first was to generate syntax sugar around existing function invokes that return CompletableFuture and wrap them using Output.all, Output.apply and Output.of(CompletableFuture) but then I saw that the SDK itself has the function that can be used immediately:

There's some leeway here, I think in most of the SDKs we went with syntactic sugar implementation, but there was some blocker making it too difficult/impossible in the C# SDK case.

Ah I remember what it was exactly. FPlainArgs and FInputArgs types differ in deeply nested Outputs. There was no way in the C# SDK to convert FInputArgs to Output<FPlainArgs> that would be required by the "syntactic sugar". And implementing the transformation at code-generation time was distasteful to me (mildly complex).

Java implementation started the port from C# so some of the same considerations can apply here.

t0yv0 · 2022-05-31T16:08:35Z

Yes, we need an update in templates repo unfortunately. Yes, the Makefile does not understand dependencies correctly which is quite frustrating. We could try working this out that'd help everyone. Providers team is experimenting and adopting with Task util instead of make also if that helps https://github.com/go-task/task

One wrench here is that the provider team is taking over the provider code trees and moving them out of the repo, so the capability to "test my changes on provider X" will have to be able to work around the source being somewhere else. I'm open to input how to make this all better, it clearly is a time sink right now.

t0yv0 · 2022-05-31T17:45:35Z

MOre review comments in #636 - since this current PR breaks GH UI

…ders check)

t0yv0 · 2022-06-03T13:51:54Z

pkg/codegen/java/gen.go

@@ -1604,31 +1596,29 @@ func (mod *modContext) genFunctions(ctx *classFileContext, addClass addClassMeth
 		// Emit the args and result types, if any.
 		if fun.Inputs != nil {
 			// Generate "{Function}Args" class for invokes that return Output<T>
+			// Notice here using fun.Inputs.InputShape.Properties which use the shape which accepting Outputs


👍

There's a bit of misnomer here. In other languages there is an Input<T> type, canonically (in TypeScript) it's an untagged union T | Output<T>. In Java we made the controversial decision to not model that type at all, so we only have Output<T> - but the cross-language Go framework is still calling that InputShape - which is fine. at the meta-level InputShape will inject InputType wrappers, that will be translated to Output<T> during the Java specific codegen pass.

t0yv0 · 2022-06-03T13:53:23Z

pkg/codegen/java/gen_test.go

@@ -35,6 +35,10 @@ var javaSpecificTests = []*test.SDKTest{
 		Directory:   "mini-awsx",
 		Description: "Regression tests extracted from trying to codegen awsx",
 	},
+	{


Hmmm, this one is a bit surprising to me. My mental model was that we borrow tests from the common test suite and then add Java-specific regression tests here. Like mini-awsnative is Java specific test. But it would seem that output-funcs-edgeorder should be coming from pulumi/pulumi? It's not there?

That is I was expecting it to be in test.PulumiPulumiSDKTests?

I'll take care of sorting this out later, not blocking.

t0yv0 · 2022-06-03T14:37:14Z

This is a lot better! The changes looks good with the following caveats:

instead of commenting out templates test I suggest we create a pulumi-java/main branch in pulumi/templates and do the edits on that branch that make the templates compatible with current changes; we then link this repo against the pulumi-java/main branch so the CI passes. When the changes are released I will merge pulumi-java/main into master on templates making it official.
it's really a pain to review. I can merge as is, but in the future, I'd like us to have the PR for review NOT include provider changes, and never mix logical changes with auto-generated test accept changes in the same commit. Under those conditions it's easier to review. We can use/perfect the script that automates generating a second PR that propagates the changes to providers, one provider per commit.

t0yv0 · 2022-06-03T14:58:12Z

Actually @Zaid-Ajaj I have tried testing the azure-template with modifications, and it's not working. Unfortunately that's blocking. We have a bug in here somewhere.

Here is the modification I've made to take advantage of the new features:

    private static Output<String> getStorageAccountPrimaryKey(Output<String> resourceGroupName,
                                                              Output<String> accountName) {
        return StorageFunctions.listStorageAccountKeys(ListStorageAccountKeysArgs.builder()
                                                       .resourceGroupName(resourceGroupName)
                                                       .accountName(accountName)
                                                       .build())
            .applyValue(r -> r.keys().get(0).value())
            .asSecret();
    }

modified: templates/azure-java/src/main/java/myproject/App.java

Here's how I tested the change:

make install_sdk
make provider.azure-native.install
make test_template.azure-java

    io.grpc.StatusRuntimeException: UNKNOWN: invocation of azure-native:storage:listStorageAccountKeys returned an error: request failed /subscriptions/0282681f-7a9e-424b-80b2-96babd57a8a1/resourceGroups/{resourceGroupName}/providers/Microsoft.Storage/storageAccounts/{accountName}/listKeys: autorest/azure: Service returned an error. Status=404 Code="ResourceGroupNotFound" Message="Resource group '{resourceGroupName}' could not be found."

t0yv0 · 2022-06-03T15:01:29Z

I guess I should update that in the templates repo and then update the templates submodule from here

Here's a reason we can't do that - updating the templates repo would BREAK the templates for all Pulumi users since these changes are not released yet. Hence my suggestion of having a branch in that repo tracking pulumi-java/main that we sync to master whenever we release.

Zaid-Ajaj · 2022-06-03T22:53:49Z

Actually @Zaid-Ajaj I have tried testing the azure-template with modifications, and it's not working. Unfortunately that's blocking. We have a bug in here somewhere.

@t0yv0 I've actually seen this error before, it is a manifestation of pulumi/pulumi-dotnet#33 where function invokes fail when populating their parameters with instances Output<T> during pulumi preview

pulumi/pulumi#9593 Also demontrates the same behaviour

The suggested workaround during a pulumi preview seems to be guarding the function invoke with an unknownOutput.apply(unused -> invokeHere()) where unknownOutput can be any ID of any resource since these unknown during a preview.

Another workaround in the template code is to guard the function invoke with an if statement that checks if we are executing a dry run

    private static Output<String> getStorageAccountPrimaryKey(Output<String> resourceGroupName,
                                                              Output<String> accountName) {

        if (Deployment.getInstance().isDryRun())
        {
             return Output.secret("*** StorageAccountPrimaryKey ***");
        } 

        return StorageFunctions.listStorageAccountKeys(ListStorageAccountKeysArgs.builder()
                                                       .resourceGroupName(resourceGroupName)
                                                       .accountName(accountName)
                                                       .build())
            .applyValue(r -> r.keys().get(0).value())
            .asSecret();
    }

The final workaround at runtime is to make function invokes smarter and expanding their logic as follows:

public static Output<ArbitFuncResult> arbitFunc(ArbitFuncArgs args)
{
    if (Deployment.getInstance().isDryRun())
    {
         // return Output<ArbitFuncResult> but it is unknown?
    }

    // execute invoke as usual... 
}

I think making the generated function invokes smarter is the way to go so that users don't have to worry changing their code to adapt to preview/update runs. What do you think?

t0yv0 · 2022-06-06T15:36:09Z

I don't believe this is the issue.

Message="Resource group '{resourceGroupName}' could not be found." failed to interpolate resourceGroupName.

How did this happen? There is a bug somewhere in Java.

t0yv0 · 2022-06-06T19:23:27Z

My preferred fix for this issue pulumi/pulumi#671 which is also what we have in C#, Python, etc.

The suggestion to not invoke in preview is interesting but I can see some downsides, well, namely that invokes don't work in preview. Bookmarking it for further discussion. This would be a change across all SDKs not just Java.

I also would like to step through the ref'd issues myself to get a better understanding:

Pulumi C# v3.2.1 is running Output.Apply in preview pulumi-dotnet#33
Output functions fail in preview section of an update if not all the inputs are available pulumi#9593

t0yv0 · 2022-06-06T19:36:04Z

Confirming that I can pass the test with pulumi/pulumi#671 merged here.

make install_sdk
make provider.azure-native.install
export PULUMI_JAVA_SDK_VERSION=0.0.1 # ouch, otherwise template test does not use the in-source SDK 
make test_template.azure-java

tests/examples/aws-java-webserver/app/src/main/java/webserver/App.java

Output-versioned function invokes

f452bb2

pawelprazak reviewed May 23, 2022

View reviewed changes

pkg/codegen/java/gen.go Outdated Show resolved Hide resolved

pkg/codegen/java/gen.go Outdated Show resolved Hide resolved

pkg/codegen/java/gen.go Outdated Show resolved Hide resolved

pawelprazak reviewed May 23, 2022

View reviewed changes

Zaid-Ajaj added 4 commits May 24, 2022 21:43

simplify propertiesAsOutputs and generate all SDKs

c3fd60c

format gen.go

0eb812b

changelog entry

46945eb

Merge branch 'main' into output-versioned-function-invokes

63a9f19

Zaid-Ajaj added 2 commits May 29, 2022 20:51

Merge branch 'main' into output-versioned-function-invokes

3f07287

test codegen changes

961afd9

Zaid-Ajaj added 2 commits May 29, 2022 22:54

attempt to fix examples: use output-versioned invokes

52ec6e1

Second attempt to fix examples

0c969f7

Zaid-Ajaj added 2 commits May 30, 2022 00:11

programgen: function invokes no londer need Output.of(...)

f2cc1a4

programgen: remove outputOf since no longer used

c60c7a3

Zaid-Ajaj marked this pull request as ready for review May 30, 2022 09:07

Zaid-Ajaj added 2 commits May 30, 2022 16:21

Bring back function invokes with completable future, disable template…

6bf7f77

…s tests

Update generated providers from test data

9529b07

t0yv0 self-requested a review May 31, 2022 15:46

Zaid-Ajaj added 5 commits June 3, 2022 14:39

remove propertiesAsOutputs flag and enable output-funcs test for codegen

1fd899c

enable output-funcs-edgeorder test

b8dc094

Add programgen example output-funcs-aws.pp

16ebebb

Merge branch 'main' into output-versioned-function-invokes

05987c7

Re-generate providers using latest codegen changes (fixes dirty provi…

c4297be

…ders check)

Zaid-Ajaj mentioned this pull request Jun 3, 2022

output versioned function invokes #636

Closed

3 tasks

t0yv0 reviewed Jun 3, 2022

View reviewed changes

t0yv0 mentioned this pull request Jun 6, 2022

Output-versioned function invoke fails to preview #670

Closed

Zaid-Ajaj added 3 commits June 6, 2022 22:47

Merge branch 'main' into output-versioned-function-invokes

3a6d868

fix aws-java-webserver example

2489642

fix import in aws-java-webserver

281f69a

t0yv0 reviewed Jun 7, 2022

View reviewed changes

tests/examples/aws-java-webserver/app/src/main/java/webserver/App.java Outdated Show resolved Hide resolved

t0yv0 added 3 commits June 7, 2022 14:39

Revert disabling template tests

cbf236a

Use tracking branch pulumi-java/main for templates refs

1984b65

Clean up CHANGELOG_PENDING

9698590

t0yv0 self-requested a review June 7, 2022 19:23

t0yv0 approved these changes Jun 7, 2022

View reviewed changes

t0yv0 merged commit 92630ce into main Jun 7, 2022

pulumi-bot deleted the output-versioned-function-invokes branch June 7, 2022 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[codegen] Output-versioned function invokes #612

[codegen] Output-versioned function invokes #612

Zaid-Ajaj commented May 23, 2022

pawelprazak left a comment •

edited

Loading

pawelprazak commented May 23, 2022

pawelprazak May 23, 2022

pawelprazak commented May 25, 2022

Zaid-Ajaj commented May 25, 2022

pawelprazak commented May 26, 2022

Zaid-Ajaj commented May 26, 2022

Zaid-Ajaj commented May 29, 2022 •

edited

Loading

Zaid-Ajaj commented May 29, 2022 •

edited

Loading

Zaid-Ajaj commented May 29, 2022

Zaid-Ajaj commented May 30, 2022 •

edited

Loading

t0yv0 commented May 31, 2022

t0yv0 commented May 31, 2022 •

edited

Loading

t0yv0 commented May 31, 2022

t0yv0 commented May 31, 2022

t0yv0 Jun 3, 2022

t0yv0 Jun 3, 2022

t0yv0 Jun 3, 2022

t0yv0 Jun 3, 2022

t0yv0 commented Jun 3, 2022

t0yv0 commented Jun 3, 2022 •

edited

Loading

t0yv0 commented Jun 3, 2022

Zaid-Ajaj commented Jun 3, 2022

t0yv0 commented Jun 6, 2022

t0yv0 commented Jun 6, 2022 •

edited

Loading

t0yv0 commented Jun 6, 2022

[codegen] Output-versioned function invokes #612

[codegen] Output-versioned function invokes #612

Conversation

Zaid-Ajaj commented May 23, 2022

Description

Checklist

pawelprazak left a comment • edited Loading

Choose a reason for hiding this comment

pawelprazak commented May 23, 2022

pawelprazak May 23, 2022

Choose a reason for hiding this comment

pawelprazak commented May 25, 2022

Zaid-Ajaj commented May 25, 2022

pawelprazak commented May 26, 2022

Zaid-Ajaj commented May 26, 2022

Zaid-Ajaj commented May 29, 2022 • edited Loading

Zaid-Ajaj commented May 29, 2022 • edited Loading

Zaid-Ajaj commented May 29, 2022

Zaid-Ajaj commented May 30, 2022 • edited Loading

t0yv0 commented May 31, 2022

t0yv0 commented May 31, 2022 • edited Loading

t0yv0 commented May 31, 2022

t0yv0 commented May 31, 2022

t0yv0 Jun 3, 2022

Choose a reason for hiding this comment

t0yv0 Jun 3, 2022

Choose a reason for hiding this comment

t0yv0 Jun 3, 2022

Choose a reason for hiding this comment

t0yv0 Jun 3, 2022

Choose a reason for hiding this comment

t0yv0 commented Jun 3, 2022

t0yv0 commented Jun 3, 2022 • edited Loading

t0yv0 commented Jun 3, 2022

Zaid-Ajaj commented Jun 3, 2022

t0yv0 commented Jun 6, 2022

t0yv0 commented Jun 6, 2022 • edited Loading

t0yv0 commented Jun 6, 2022

pawelprazak left a comment •

edited

Loading

Zaid-Ajaj commented May 29, 2022 •

edited

Loading

Zaid-Ajaj commented May 29, 2022 •

edited

Loading

Zaid-Ajaj commented May 30, 2022 •

edited

Loading

t0yv0 commented May 31, 2022 •

edited

Loading

t0yv0 commented Jun 3, 2022 •

edited

Loading

t0yv0 commented Jun 6, 2022 •

edited

Loading