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 8322 #8339

Merged
merged 12 commits into from
Nov 8, 2021
Merged

Fix 8322 #8339

merged 12 commits into from
Nov 8, 2021

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Nov 2, 2021

Description

Fixes #8322

In other languages like TypeScript, we implement the Output-versioned invokes using Apply. This means in preview, if any arguments are unknown, monitor is not called at all (short-circuit). This was missing in C#, causing breaking previews.

I believe this works but this continues to be a bit awkward at implementation level. Relative to TypeScript and Python, .NET SDK feels a bit lacking in power:

  • cannot implement this feature using Apply as it's unclear that the SDK has power to transform inputShape(Args) to Input(Args)

  • therefore emulating Apply features indirectly in the serialization related machinery

  • decided against detecting unknowns on the native representation (input to the serializer), as doing so involves peeking under lazy Inputs and potentially triggering side-effects, and also it seems a bit tricky to recognize all values correctly

  • therefore opted to detecting unknowns AFTER serialization in the serialized form, before it gets projected into protobuf form; this seems to be well defined, but needs eyes as the not supported by type system (object? trees)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@t0yv0 t0yv0 requested review from justinvp, mikhailshilkov and a team November 2, 2021 21:54
// Wait for all values to be available, and then perform the RPC.
var argsDict = await args.ToDictionaryAsync().ConfigureAwait(false);

var keepResources = await this.MonitorSupportsResourceReferences().ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: slight reordering here, in that MonitorSupportsResourceReferences call will now go before args.ToDictionaryAsync() await, and before Log.Debug(label); hopefully this is benign.

@@ -85,5 +96,24 @@ public partial class Deployment
propertyToDependentResources = PropertyToDependentResources;
}
}

private readonly struct RawSerializationResult
Copy link
Member Author

Choose a reason for hiding this comment

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

Helper.. Alternatively can go for SerializationResult, or tuples.

@pgavlin
Copy link
Member

pgavlin commented Nov 2, 2021

cannot implement this feature using Apply as it's unclear that the SDK has power to transform inputShape(Args) to Input(Args)

This has come up before, but I want to double-check something for my own understanding: does this mean that without output-shaped functions, .NET users cannot use Apply to achieve the same result b/c the C# type system can't express what we want?

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 3, 2021

This has come up before, but I want to double-check something for my own understanding: does this mean that without output-shaped functions, .NET users cannot use Apply to achieve the same result b/c the C# type system can't express what we want?

The users can always use Apply to emulate fn.Output form by coding the transformation from inputs to Args. That is always possible. However they need to code the transformation in each specific case separately, akin to this:

Tuple(x, y, z).Apply(t =>
{
   GetAmiIds.InvokeAsync(new GetAmiIdsArgs{ X = t.Item1, Y = t.Item2, Z = t.Item3 })
})

What is limited is our generic programming model, not the specific programming model. We're having difficulty here writing code that will work correctly for all possible concrete Args types. We can't easily take the above code and make it abstract over GetAmiIdsArgs.

In TypeScript, the advanced type system takes care of it by being able to type-check the generic programs.

However this is not the only way. In languages with simpler (or no) type systems, reflective programming achieves the same effect.

Basically if we had something like this:

PlainShape<T1,T2>(T1 term) Input<T2> 
PlainShapeReflective(Type t1, Type t2, object term) object // or this

Then we could implement like this:

public Task<GetAmiIdsResult> InvokeAsync(GetAmiIdsArgs args, InvokeOptions? options = null)

public Output<GetAmiIdsResult> Invoke(GetAmiIdsInvokeArgs args, InvokeOptions? options = null)
{
   PlainShape<GetAmiIdsInvokeArgs, GetAmiIdsArgs>(args).Apply(options => InvokeAsync(args, options))
}

Which I think would be cleaner, definitely avoid bugs like the one fixed here. However I think .NET SDK does not yet know how to do something like PlainShape, and it's a bit of work to teach it.

@pgavlin
Copy link
Member

pgavlin commented Nov 3, 2021

The users can always use Apply to emulate fn.Output form by coding the transformation from inputs to Args. That is always possible. However they need to code the transformation in each specific case separately, akin to this:

Okay, this is very helpful. Thanks for clarifying!

Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

I'm still a bit hesitant about how we handle this in .NET (relying on serialization layer rather than Apply like the other languages), but I understand the reasons.

Some comments/suggestions, but otherwise LGTM.

sdk/dotnet/Pulumi.Tests/Mocks/MocksTests.cs Outdated Show resolved Hide resolved
sdk/dotnet/Pulumi/Deployment/Deployment_Serialization.cs Outdated Show resolved Hide resolved
sdk/dotnet/Pulumi/Serialization/Serializer.cs Outdated Show resolved Hide resolved
sdk/dotnet/Pulumi/Serialization/Serializer.cs Outdated Show resolved Hide resolved
sdk/dotnet/Pulumi/Deployment/Deployment_Invoke.cs Outdated Show resolved Hide resolved
sdk/dotnet/Pulumi/Deployment/Deployment_Invoke.cs Outdated Show resolved Hide resolved
t0yv0 and others added 3 commits November 5, 2021 12:13
Co-authored-by: Justin Van Patten <jvp@justinvp.com>
@t0yv0 t0yv0 merged commit e37892a into master Nov 8, 2021
@pulumi-bot pulumi-bot deleted the t0yv0/fix-8322 branch November 8, 2021 15:45
abhinav pushed a commit to pulumi/pulumi-dotnet that referenced this pull request Jan 11, 2023
* Fix 8322

* Untabify

* Untabify again

* Yet More untabify

* More untabify

* Final untabify

* Add CHANGELOG_PENDING

* Apply suggestions from code review

Co-authored-by: Justin Van Patten <jvp@justinvp.com>

* PR feedback

Co-authored-by: Justin Van Patten <jvp@justinvp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output-versioned function invoke fails to preview in C#/.NET
3 participants