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

Send output values to transforms for dependency tracking #15637

Merged
merged 2 commits into from Mar 20, 2024

Conversation

Frassle
Copy link
Member

@Frassle Frassle commented Mar 12, 2024

Description

This engine change is necessary for correct dependency tracking of properties through transform functions.

Unlike other parts of the runtime/provider/engine interface transform functions do not have a property dependencies map sent or returned. Transforms rely entirely on the dependency arrays in output values for their dependency tracking.

This change takes the property dependency map and upgrades all the input properties to be output values tracking those same dependencies (if a property doesn't have any dependencies it doesn't need to be upgraded to an output value).
This upgrade is only done if we're using transform functions, there's currently no need to do this unless we're using transforms.

After running the transforms we downgrade the output values back to plain/secret/computed values if we're registering a custom resource. If we're registering a component resource we just leave all the properties upgraded as output values.
We also rebuild the resources dependencies slice and propertyDependency map with the dependencies recorded in the transformed properties. If the transform didn't change the properties this will just be the same data we encoded into the output values in the properties structure.

There are a couple of things to keep in mind with this change.

Firstly using a transform can cause a property that was being sent to a component provider as just a resource.NumberProperty to start being sent as an OutputProperty with the NumberProperty as its element. Component providers should handle that, but it's feasible that it could break some component code. This is a fairly limited blast radius as it will only happen when that user starts using a transform function.

Secondly, we can't know in the engine if a property dependencies are from a top level output value or a nested output. That is SDKs send both of the following property shapes the same way to the engine for custom resources:

A) Output(Array([Number(1)]), ["dep"])
---
B) Array([Output(Number(1), ["dep"])

Currently both would get upgraded to Output(Array([Number(1)]), ["dep"]). For a component resource the SDK will send output values, and as long as they define a superset of the dependencies in the property map then we don't add the top-level output tracking the same dependency set.

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@Frassle Frassle force-pushed the fraser/transformOutputs branch 2 times, most recently from 7c63df9 to fe1a595 Compare March 12, 2024 00:43
@pulumi-bot
Copy link
Contributor

pulumi-bot commented Mar 12, 2024

Changelog

[uncommitted] (2024-03-19)

Features

  • [engine] Send output values with property dependency information to transform functions
    #15637

@Frassle Frassle force-pushed the fraser/transformOutputs branch 6 times, most recently from e625a63 to 5f33588 Compare March 13, 2024 17:15
@Frassle Frassle changed the title working on upgrading output values Send output values to transforms for dependency tracking Mar 13, 2024
@Frassle
Copy link
Member Author

Frassle commented Mar 13, 2024

This needs a bit more set handling for the dependency arrays, going to try and get #15671 and maybe some other small changes in first to minimise diff size.

@Frassle Frassle force-pushed the fraser/transformOutputs branch 5 times, most recently from 4d25406 to 5bf73d9 Compare March 16, 2024 08:24
@Frassle Frassle marked this pull request as ready for review March 16, 2024 13:03
@Frassle Frassle requested a review from a team as a code owner March 16, 2024 13:03
Copy link
Collaborator

@tgummerer tgummerer left a comment

Choose a reason for hiding this comment

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

This looks good go me, with one question inline.

} else {
// If we ran transforms we would have merged all the dependencies togther already, but if we didn't we want to
// ensure any output values add their dependencies to the dependencies map we send to the provider.
for key, output := range props {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I'm following this change properly. Since we didn't upgrade the props in the case where there are no transforms, couldn't we just use the old code from https://github.com/pulumi/pulumi/pull/15637/files#diff-984b3279d61b588030474831898bb76d093d863ca2d0015bb081d842602a9ab7L1665? Or why is this change necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is pretty much that code but for Set instead of slice.

@justinvp
Copy link
Member

Firstly using a transform can cause a property that was being sent to a component provider as just a resource.NumberProperty to start being sent as an OutputProperty with the NumberProperty as its element.

Does this only happen if that property has dependencies?

@Frassle
Copy link
Member Author

Frassle commented Mar 19, 2024

Does this only happen if that property has dependencies?

Yes

@Frassle Frassle added this pull request to the merge queue Mar 20, 2024
Merged via the queue into master with commit f749c2c Mar 20, 2024
48 checks passed
@Frassle Frassle deleted the fraser/transformOutputs branch March 20, 2024 10:44
github-merge-queue bot pushed a commit that referenced this pull request Mar 27, 2024
Tentative changelog:

### Features

- [docs] Implement constructor syntax examples for every resource in
typescript, python, csharp and go
  [#15624](#15624)

- [engine] Send output values with property dependency information to
transform functions
  [#15637](#15637)

- [engine] Add a --continue-on-error flag to pulumi destroy
  [#15727](#15727)

- [sdk/go] Make `property.Map` keyed by `string` not `MapKey`
  [#15767](#15767)

- [sdk/python] Improve the error message when depends_on is passed
objects of the wrong type
  [#15737](#15737)


### Bug Fixes

- [auto/{go,nodejs,python}] Make sure to read complete lines before
trying to deserialize them as engine events
  [#15778](#15778)

- [cli/plugin] Fix installing local language plugins on Windows
  [#15715](#15715)

- [engine] Don't delete stack outputs on failed deployments
  [#15754](#15754)

- [engine] Fix a panic when updating provider version in a run using
--target
  [#15716](#15716)

- [engine] Handle that Assets & Archives can be returned from providers
without content.
  [#15736](#15736)

- [engine] Fix the engine trying to delete a protected resource caught
in a replace chain
  [#15776](#15776)

- [sdkgen/docs] Add missing newline for `Coming soon!`
  [#15783](#15783)

- [programgen/dotnet] Fix generated code for a list of resources used in
resource option DependsOn
  [#15773](#15773)

- [programgen/{dotnet,go}] Fixes emitted code for object expressions
assigned to properties of type Any
  [#15770](#15770)

- [sdk/go] Fix lookup of plugin and program dependencies when using Go
workspaces
  [#15743](#15743)

- [sdk/nodejs] Export automation.tag.TagMap type
  [#15774](#15774)

- [sdk/python] Wait only for pending outputs in the Python SDK, not all
pending asyncio tasks
  [#15744](#15744)


### Miscellaneous

- [sdk/nodejs] Reorganize function serialization tests
  [#15753](#15753)

- [sdk/nodejs] Move mockpackage tests to closure integration tests
  [#15757](#15757)
@justinvp justinvp mentioned this pull request Mar 27, 2024
github-merge-queue bot pushed a commit that referenced this pull request Mar 28, 2024
Tentative changelog:

### Features

- [docs] Implement constructor syntax examples for every resource in
typescript, python, csharp and go
  [#15624](#15624)

- [docs] Implement YAML constructor syntax examples in the docs
  [#15791](#15791)

- [engine] Send output values with property dependency information to
transform functions
  [#15637](#15637)

- [engine] Add a --continue-on-error flag to pulumi destroy
  [#15727](#15727)

- [sdk/go] Make `property.Map` keyed by `string` not `MapKey`
  [#15767](#15767)

- [sdk/nodejs] Make function serialization work with typescript 4 and 5
  [#15761](#15761)

- [sdk/python] Improve the error message when depends_on is passed
objects of the wrong type
  [#15737](#15737)


### Bug Fixes

- [auto/{go,nodejs,python}] Make sure to read complete lines before
trying to deserialize them as engine events
  [#15778](#15778)

- [cli/plugin] Fix installing local language plugins on Windows
  [#15715](#15715)

- [engine] Don't delete stack outputs on failed deployments
  [#15754](#15754)

- [engine] Fix a panic when updating provider version in a run using
--target
  [#15716](#15716)

- [engine] Handle that Assets & Archives can be returned from providers
without content.
  [#15736](#15736)

- [engine] Fix the engine trying to delete a protected resource caught
in a replace chain
  [#15776](#15776)

- [sdkgen/docs] Add missing newline for `Coming soon!`
  [#15783](#15783)

- [programgen/dotnet] Fix generated code for a list of resources used in
resource option DependsOn
  [#15773](#15773)

- [programgen/{dotnet,go}] Fixes emitted code for object expressions
assigned to properties of type Any
  [#15770](#15770)

- [sdk/go] Fix lookup of plugin and program dependencies when using Go
workspaces
  [#15743](#15743)

- [sdk/nodejs] Export automation.tag.TagMap type
  [#15774](#15774)

- [sdk/python] Wait only for pending outputs in the Python SDK, not all
pending asyncio tasks
  [#15744](#15744)


### Miscellaneous

- [sdk/nodejs] Reorganize function serialization tests
  [#15753](#15753)

- [sdk/nodejs] Move mockpackage tests to closure integration tests
  [#15757](#15757)
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.

None yet

4 participants