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

[sdk/python] Add an invoke_async function #15602

Merged
merged 1 commit into from Mar 6, 2024
Merged

Conversation

justinvp
Copy link
Member

@justinvp justinvp commented Mar 5, 2024

This commit adds a new pulumi.runtime.invoke_async function that allows calling invokes asynchronously. The initial intended use for this is inside the Kubernetes Python SDK's yaml.ConfigFile, yaml.ConfigGroup, helm, kustomize, etc. components to avoid stalls in resource registrations which severely limits parallelism.

I'd love to add some kind of benchmark, perhaps along the lines of the repro in #15462, but we can add that as a fast-follow.

Part of #15462 and #15591

After this is merged and released, we can ship an updated version of the Kubernetes Python SDK that makes use of it.

This commit adds a new `pulumi.runtime.invoke_async` function that allows calling invokes asynchronously. The initial intended use for this is inside the Kubernetes Python SDK's `yaml.ConfigFile`, `yaml.ConfigGroup`, `helm`, `kustomize`, etc. components to avoid stalls in resource registrations which severely limits parallelism.
@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2024-03-05)

Features

  • [sdk/python] Add support for asynchronous invokes via a new invoke_async function
    #15602

@justinvp justinvp marked this pull request as ready for review March 5, 2024 23:00
@justinvp justinvp requested a review from a team as a code owner March 5, 2024 23:00
props: "Inputs",
opts: Optional[InvokeOptions] = None,
typ: Optional[type] = None,
) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

this is probably the right signature, but it is a little odd that this isn't returning an InvokeResult. returning an InvokeResult would allow callees of invoke to transition to invoke_async by just changing the function name and adding await.

Copy link
Member Author

@justinvp justinvp Mar 5, 2024

Choose a reason for hiding this comment

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

Ah, I had added some comments to the PR before I tagged you, but I guess I never clicked "submit" on those. One of the comments touched on this. Went ahead and submitted those.

In any case, yeah, I went back and forth on this. It doesn't strictly need to be InvokeResult -- our other SDKs like Node don't do this, they just return Promise<T>. And originally the Python SDK didn't until we reworked the previous async invokes to be synchronous.

Copy link
Member Author

Choose a reason for hiding this comment

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

One other thing... I don't really expect much use of this directly by users. This is a pretty advanced use case. The vast majority of users are going to be using generated provider SDK invokes. So not sure we need to optimize for avoiding having to remove a .value when transitioning. I almost lean towards the "right" signature in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me 👍

return _invoke(tok, props, opts, typ, run_async=False)


async def invoke_async(
Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than invoke_internal, I think it's reasonable to go ahead and expose this in the core SDK as invoke_async. Curious if folks feel strongly otherwise. Initially it'll only be used by those hand-authored Kubernetes components, but we may want to use it for async invoke variants of generated provider SDKs in the future.

the inputs can be a bag of computed values (Ts or Awaitable[T]s), and the result is a Awaitable[Any] that
resolves when the invoke finishes.
"""
return await _invoke(tok, props, opts, typ, run_async=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, this doesn't need to be an async function that does this await. It could be:

def invoke_async(
    tok: str,
    props: "Inputs",
    opts: Optional[InvokeOptions] = None,
    typ: Optional[type] = None,
) -> Coroutine[Any, Any, Any]:
    return _invoke(tok, props, opts, typ, run_async=True)

It just somehow feels cleaner as the async func, as then I don't have to explicitly specify Coroutine[Any, Any, Any] as the return type hint.

Comment on lines +132 to +154
@overload
def _invoke(
tok: str,
props: "Inputs",
opts: Optional[InvokeOptions],
typ: Optional[type],
run_async: Literal[False],
) -> InvokeResult: ...
@overload
def _invoke(
tok: str,
props: "Inputs",
opts: Optional[InvokeOptions],
typ: Optional[type],
run_async: Literal[True],
) -> Coroutine[Any, Any, Any]: ...
def _invoke(
tok: str,
props: "Inputs",
opts: Optional[InvokeOptions],
typ: Optional[type],
run_async: bool,
):
Copy link
Member Author

Choose a reason for hiding this comment

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

It's kind of annoying to have these overloads, but it does make the return type hinting better.

  • When run_async=False it returns InvokeResult.
  • When run_async=True it returns Coroutine[Any, Any, Any] (basically Awaitable[Any])

If I don't do this, then I'd either have to ignore the type check on the return wait_for_fut() or do something else to make the type checker happy.

invoke_result, invoke_error = await fut
if invoke_error is not None:
raise invoke_error
return invoke_result
Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than wrapping the result in InvokeResult, I'm proposing we return the result directly for the async case.

@justinvp justinvp added this pull request to the merge queue Mar 6, 2024
Merged via the queue into master with commit 73e2471 Mar 6, 2024
48 checks passed
@justinvp justinvp deleted the justin/invoke_async branch March 6, 2024 01:28
github-merge-queue bot pushed a commit that referenced this pull request Mar 6, 2024
### Features

- [pkg] Make schema.NewPluginLoader respect PULUMI_DEBUG_PROVIDERS,
which enables Pulumi YAML programs to work correctly with this feature
  [#15526](#15526)

- [sdk/python] Add support for asynchronous invokes via a new
`invoke_async` function
  [#15602](#15602)

- [sdkgen/dotnet] Support for non-overlay components in codegen for
pulumi-kubernetes provider
  [#15490](#15490)


### Bug Fixes

- [backend/service] Make decrypt/encrypt network calls retryable to help
work around network hiccups
  [#15600](#15600)

- [cli/new] Strip credentials and query strings from template URLs saved
to project
  [#15586](#15586)

- [engine] Fix an issue where snapshots could become invalid when doing
a targeted up
  [#15476](#15476)

- [sdk/python] Fix determining plugins for old packages in the Python
language host
  [#15576](#15576)

- [pkg/testing] Make ProgramTest use a temporary PULUMI_HOME for each
test
  [#15568](#15568)

- [sdkgen/dotnet] Codegen fix for resources without constant input
properties
  [#15488](#15488)
github-merge-queue bot pushed a commit that referenced this pull request Mar 7, 2024
Draft changelog:

### Features

- [auto/{go,nodejs,python}] Add support for suppress progress and
suppress outputs parameters in the Automation API
  [#15596](#15596)

- [pkg] Make schema.NewPluginLoader respect PULUMI_DEBUG_PROVIDERS,
which enables Pulumi YAML programs to work correctly with this feature
  [#15526](#15526)

- [sdk/dotnet] Update dotnet language host to 3.60.0
  [#15609](#15609)

- [sdk/nodejs] Add experimental support to the NodeJS SDK for the new
transforms system.
  [#15532](#15532)

- [sdk/python] Add support for asynchronous invokes via a new
`invoke_async` function
  [#15602](#15602)

- [sdkgen/dotnet] Support for non-overlay components in codegen for
pulumi-kubernetes provider
  [#15490](#15490)


### Bug Fixes

- [cli] Fix a panic when the secrets provider is missing from the
deployment snapshot
  [#15599](#15599)

- [backend/service] Make decrypt/encrypt network calls retryable to help
work around network hiccups
  [#15600](#15600)

- [cli/new] Strip credentials and query strings from template URLs saved
to project
  [#15586](#15586)

- [engine] Fix an issue where snapshots could become invalid when doing
a targeted up
  [#15476](#15476)

- [pkg/testing] Make ProgramTest use a temporary PULUMI_HOME for each
test
  [#15568](#15568)

- [sdkgen/dotnet] Codegen fix for resources without constant input
properties
  [#15488](#15488)

- [sdk/nodejs] Properly capture node:crypto and global.crypto in node
19+

- [sdk/python] Fix determining plugins for old packages in the Python
language host
  [#15576](#15576)
justinvp added a commit to pulumi/pulumi-kubernetes that referenced this pull request Mar 7, 2024
pulumi 3.109.0 introduces a new `invoke_async` function
(pulumi/pulumi#15602) that allows calling
invokes asynchronously. This change updates the Kubernetes Python SDK's
`yaml`, `helm`, and `kustomize` components to use `invoke_async` to
avoid hangs and stalls in resource registrations which severely limits
parallelism.

It'd also be great to add some new tests (to complement existing tests),
but I don't want to block on getting a fix out to customers:
1. A test similar to the repro of the hang in
pulumi/pulumi#15462. It's difficult to repro
this without mocking, though.
2. Some kind of benchmark that demonstrates this improves performance.

Fixes pulumi/pulumi#15462
Fixes pulumi/pulumi#15591
rquitales pushed a commit to pulumi/pulumi-kubernetes that referenced this pull request Mar 13, 2024
pulumi 3.109.0 introduces a new `invoke_async` function
(pulumi/pulumi#15602) that allows calling
invokes asynchronously. This change updates the Kubernetes Python SDK's
`yaml`, `helm`, and `kustomize` components to use `invoke_async` to
avoid hangs and stalls in resource registrations which severely limits
parallelism.

It'd also be great to add some new tests (to complement existing tests),
but I don't want to block on getting a fix out to customers:
1. A test similar to the repro of the hang in
pulumi/pulumi#15462. It's difficult to repro
this without mocking, though.
2. Some kind of benchmark that demonstrates this improves performance.

Fixes pulumi/pulumi#15462
Fixes pulumi/pulumi#15591

(cherry picked from commit e174e29)
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

3 participants