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

Support Python 3.12 #15190

Merged
merged 5 commits into from Jan 25, 2024
Merged

Support Python 3.12 #15190

merged 5 commits into from Jan 25, 2024

Conversation

justinvp
Copy link
Member

@justinvp justinvp commented Jan 19, 2024

Python 3.12 requires grpcio 1.59.0 or higher. Unfortunately, there is a regression in grpcio 1.58.0 through the latest version (currently 1.60.0) which causes any error returned from a Python gRPC server to be written to stderr, including UNIMPLEMENTED errors. This primarily affects Python dynamic providers, which don't have implementations for CheckConfig and DiffConfig, resulting in a traceback error being emitted to stderr when the engine calls these, which is visible to users. This grpcio regression has been fixed upstream, but the fix has not been released yet. We've been waiting for a 1.60.1 patch release.

This has not been great for our Python users who are using Python 3.12. It's particularly bad for new Pulumi users who are using Python 3.12 and are trying to get started with Pulumi. For these users, when trying to install the pulumi PyPi package (i.e. via pulumi new python) the installation fails with an error because it is pinned to depending on an older version of grpcio which doesn't work on Python 3.12.

This commit works around the problem by providing default implementations of CheckConfig and DiffConfig for python dynamic providers and the component provider API, so that no error is emitted to stderr when the engine calls these methods. The default implementations for these are the same behavior that the engine would use if these methods had returned UNIMPLEMENTED. I believe these are the only two methods affected by this. Other methods like Invoke, Call, StreamInvoke, Construct, Attach, GetMapping, and GetMappings, continue to return UNIMPLEMENTED for dynamic providers, which I think is OK; I don't believe these will be called by the engine under normal circumstances.

Out of an abundance of caution, the pulumi package continues to depend on the pinned version of grpcio when installing on versions of Python less than 3.12. On Python 3.12 or greater, we now depend on grpcio ~=1.60.0. 1.60.0 doesn't have the fix for the regression, but the workaround should allow things to work on Python 3.12 as before.

Once 1.60.1 is released, we can look into updating the grpcio dependency to ~=1.60.1 for all versions of Python, and possibly revert the workarounds, if we want.

Note: #14474 added a test for dynamic providers to ensure nothing is written to stderr. The test would fail if the workaround in this PR did not work as intended: https://github.com/pulumi/pulumi/pull/14474/files#diff-d92ccd283e08eadab2597825103e45cdaa96fea93324bc4d4d3b1d2b83c51b76

This PR depends on several other smaller PRs:

Fixes #14258

@pulumi-bot
Copy link
Contributor

pulumi-bot commented Jan 19, 2024

Changelog

[uncommitted] (2024-01-24)

Features

  • [sdk/python] Add support for Python 3.12
    #15190

Comment on lines +61 to +69
# CheckConfig is not implemented by the dynamic provider.
# Just return the news as if we're OK with them, which is the default behavior.
#
# Note: We're not using `context.set_code(grpc.StatusCode.UNIMPLEMENTED)` as grpcio
# 1.58.0 through 1.60.0 has a bug that causes the UNIMPLEMENTED error to be written
# to stderr, which users will see. 1.60.1 should include a fix for the stderr
# regression, but it hasn't been released yet. Once 1.60.1 is released, we can
# depend on that version and go back to returning UNIMPLEMENTED.
return proto.CheckResponse(inputs=request.news)
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 matches the behavior of the engine if this is UNIMPLEMENTED:

if code == codes.Unimplemented || isDiffCheckConfigLogicallyUnimplemented(rpcError, urn.Type()) {
// For backwards compatibility, just return the news as if the provider was okay with them.
logging.V(7).Infof("%s unimplemented rpc: returning news as is", label)
return news, nil, nil
}

Comment on lines +72 to +80
# DiffConfig is not implemented by the dynamic provider.
# Just return DIFF_UNKNOWN, which is the default behavior.
#
# Note: We're not using `context.set_code(grpc.StatusCode.UNIMPLEMENTED)` as grpcio
# 1.58.0 through 1.60.0 has a bug that causes the UNIMPLEMENTED error to be written
# to stderr, which users will see. 1.60.1 should include a fix for the stderr
# regression, but it hasn't been released yet. Once 1.60.1 is released, we can
# depend on that version and go back to returning UNIMPLEMENTED.
return proto.DiffResponse(changes=proto.DiffResponse.DIFF_UNKNOWN)
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 matches the behavior of the engine if this is UNIMPLEMENTED:

if code == codes.Unimplemented || isDiffCheckConfigLogicallyUnimplemented(rpcError, urn.Type()) {
logging.V(7).Infof("%s unimplemented rpc: returning DiffUnknown with no replaces", label)
// In this case, the provider plugin did not implement this and we have to provide some answer:
//
// There are two interesting scenarios with the present gRPC interface:
// 1. Configuration differences in which all properties are known
// 2. Configuration differences in which some new property is unknown.
//
// In both cases, we return a diff result that indicates that the provider _should not_ be replaced.
// Although this decision is not conservative--indeed, the conservative decision would be to always require
// replacement of a provider if any input has changed--we believe that it results in the best possible user
// experience for providers that do not implement DiffConfig functionality. If we took the conservative
// route here, any change to a provider's configuration (no matter how inconsequential) would cause all of
// its resources to be replaced. This is clearly a bad experience, and differs from how things worked prior
// to first-class providers.
return DiffResult{Changes: DiffUnknown, ReplaceKeys: nil}, nil

@justinvp
Copy link
Member Author

justinvp commented Jan 19, 2024

Looks like there are some issues when running CI with Python 3.12:

  • pylint issue
  • Issue with setuptools no longer being included in virtual environments created with python -m venv

Edit: Fixed

@justinvp
Copy link
Member Author

More test failures to investigate.

@justinvp justinvp force-pushed the justin/python3.12 branch 2 times, most recently from 7619361 to c337328 Compare January 23, 2024 23:31
@justinvp justinvp marked this pull request as ready for review January 23, 2024 23:34
@justinvp justinvp requested a review from a team January 23, 2024 23:34
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.

I do think we should revert the workarounds once we can update to 1.60.1, but this is a good step forward in the meantime!

Python 3.12 requires `grpcio` 1.59.0 or higher. Unfortunately, there is
a regression in `grpcio` 1.58.0 through the latest version (currently
1.60.0) which causes any error returned from a Python gRPC server to be
written to stderr, including UNIMPLEMENTED errors. This primarily
affects Python dynamic providers, which don't have implementations for
`CheckConfig` and `DiffConfig`, resulting in a traceback error being
emitted to stderr when the engine calls these, which is visible to
users. This `grpcio` regression has been fixed upstream, but the fix has
not been released yet. We've been waiting for a 1.60.1 patch release.
There is a pre-release version of 1.60.1 on GitHub, but it's not
available on PyPi.

This has not been great for our Python users who are using Python 3.12.
It's particularly bad for new Pulumi users who are using Python 3.12 and
are trying to get started with Pulumi. For these users, when trying to
install the `pulumi` PyPi package (i.e. via `pulumi new python`) the
installation fails with an error because it is pinned to depending on an
older version of `grpcio` which doesn't work on Python 3.12.

This commit works around the problem by providing default
implementations of `CheckConfig` and `DiffConfig` for python dynamic
providers and the component provider API, so that no error is emitted to
stderr when the engine calls these methods. The default implementations
for these are the same behavior that the engine would use if these
methods had returned UNIMPLEMENTED. I believe these are the only two
methods affected by this. Other methods like `Invoke`, `Call`,
`StreamInvoke`, `Construct`, `Attach`, `GetMapping`, and `GetMappings`,
continue to return UNIMPLEMENTED for dynamic providers, which I think is
OK; I don't believe these will be called by the engine under normal
circumstances.

Out of an abundance of caution, the `pulumi` package continues to depend
on the pinned version of `grpcio` when installing on versions of Python
less than 3.12. On Python 3.12 or greater, we now depend on `grpcio`
`~=1.60.0`. 1.60.0 doesn't have the fix for the regression, but the
workaround should allow things to work on Python 3.12 as before.

Once 1.60.1 is released, we can look into updating the `grpcio`
dependency to `~=1.60.1` for all versions of Python, and possibly revert
the workarounds, if we want.
This test requires the `pulumi-tls` package, which currently depends on the published version of `pulumi` which can't be installed on Python 3.12 due to it requiring an older version of `grpcio` which can't be installed on Python 3.12.

The test does install the locally built Pulumi Python SDK, but `ProgramTest` does that _after_ installing any dependencies specified in the test program's `requirements.txt`.

We probably should consider updating `ProgramTest` to install the local dependencies first, or providing a new option to do that.

But in the meantime, the simplest workaround is to temporarily skip this test. After we've released a new version of `pulumi` that can be installed on Python 3.12, we can re-enable the test.
This test depends on the published version of `pulumi` which can't be installed on Python 3.12 due to it requiring an older version of `grpcio` which can't be installed on Python 3.12.

For now, temporarily skip this test. After we've released a new version of `pulumi` that can be installed on Python 3.12, we can re-enable the test.
This test depends on the published version of `pulumi` which can't be installed on Python 3.12 due to it requiring an older version of `grpcio` which can't be installed on Python 3.12.

For now, temporarily skip this test. After we've released a new version of `pulumi` that can be installed on Python 3.12, we can re-enable the test.
@justinvp justinvp mentioned this pull request Jan 24, 2024
These tests depends on the published version of `pulumi` which can't be installed on Python 3.12 due to it requiring an older version of `grpcio` which can't be installed on Python 3.12.

For now, temporarily skip these tests. After we've released a new version of `pulumi` that can be installed on Python 3.12, we can re-enable the test.
@justinvp justinvp added this pull request to the merge queue Jan 24, 2024
Merged via the queue into master with commit 7309681 Jan 25, 2024
46 checks passed
@justinvp justinvp deleted the justin/python3.12 branch January 25, 2024 00:19
justinvp added a commit that referenced this pull request Feb 3, 2024
grpcio 1.60.1 includes a fix for the regression that results in a traceback being written to stderr for unimplemented gRPC methods. https://github.com/grpc/grpc/releases/tag/v1.60.1

This change upgrades to 1.60.1 and reverts the workarounds we put in place in #15190.

Note that we have an integration test for Python dynamic providers that ensures nothing unexpected is written to stderr: https://github.com/pulumi/pulumi/blob/16a1fb31c2040785dbbfb9de43d772d8e16fffaa/tests/integration/integration_python_acceptance_test.go#L111-L117
github-merge-queue bot pushed a commit that referenced this pull request Feb 5, 2024
grpcio 1.60.1 includes a fix for the regression that results in a
traceback being written to stderr for unimplemented gRPC methods.
https://github.com/grpc/grpc/releases/tag/v1.60.1

This change upgrades to 1.60.1 and reverts the workarounds we put in
place in #15190.

Note that we have an integration test for Python dynamic providers that
ensures nothing unexpected is written to stderr:
https://github.com/pulumi/pulumi/blob/16a1fb31c2040785dbbfb9de43d772d8e16fffaa/tests/integration/integration_python_acceptance_test.go#L111-L117

Fixes #15365
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.

Installation is broken on Python 3.12
4 participants