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

Make Output[T] covariant in Python SDK #7483

Merged
merged 1 commit into from
Jul 16, 2021
Merged

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Jul 10, 2021

In short, this allows subtyping to correctly "propagate" through an Output;
if Cow is a subtype of Animal, this commit makes it so Output[Cow] is
treated as a subtype of Output[Animal].

Without this change, users of the Python SDK occasionally contend with a
confusing error message that is resolved by adding a call to
.apply(lambda x: x) to satisfy mypy.

Touches #3767.
Fix #6843.

cc @lukehoban @t0yv0

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

Further commands available:

  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

In short, this allows subtyping to correctly "propagate" through an Output;
if Cow is a subtype of Animal, this commit makes it so Output[Cow] is
treated as a subtype of Output[Animal].

Without this change, users of the Python SDK occasionally contend with a
confusing error message that is resolved by adding a call to
`.apply(lambda x: x)` to satisfy mypy.

Touches pulumi#3767.
Fix pulumi#6843.
@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

Further commands available:

  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@benesch
Copy link
Contributor Author

benesch commented Jul 14, 2021 via email

@orionstudt
Copy link
Contributor

@benesch Makes sense for python. I don't think it is already covariant in the dotnet SDK. Sorry I deleted my comment though, thought it made more sense to make it on the issue instead of your PR

@komalali
Copy link
Member

/run-acceptance-tests

@github-actions
Copy link

Please view the results of the PR Build + Acceptance Tests Run Here

@komalali komalali requested review from justinvp, t0yv0 and pgavlin July 16, 2021 17:48
Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

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

LGTM.

Should we also add a contravariant type variable for Inputs in a future change? cc @lukehoban @t0yv0 @komalali @justinvp

@pgavlin pgavlin merged commit c782a29 into pulumi:master Jul 16, 2021
@benesch benesch deleted the output-covar branch July 16, 2021 19:50
@t0yv0
Copy link
Member

t0yv0 commented Jul 16, 2021

I was slightly scared to recommend going ahead with this as I don't feel I have a good handle on all the implications, but I guess we will find out now that it is in :)

Input[T] and Output[T] should both have the same variance (if any) imo, that'd be covariant, because T occurs in a positive position in both (contrast with a negative position in a function type e.g. T -> int).

@lukehoban
Copy link
Contributor

I was slightly scared to recommend going ahead with this as I don't feel I have a good handle on all the implications, but I guess we will find out now that it is in :)

Are we running mypy as part of our build to catch any possible impacts of this?

@benesch
Copy link
Contributor Author

benesch commented Jul 16, 2021

Input[T] and Output[T] should both have the same variance (if any) imo, that'd be covariant, because T occurs in a positive position in both (contrast with a negative position in a function type e.g. T -> int).

Agree! pulumi.Input[T] is already working correctly (covariantly) by my read.

Are we running mypy as part of our build to catch any possible impacts of this?

The python/sdk library definitely gets a mypy run via the lint target. Not sure about downstream providers though.

@pgavlin
Copy link
Member

pgavlin commented Jul 16, 2021

Agree! pulumi.Input[T] is already working correctly (covariantly) by my read.

Perfect. Covariance and contravariance is something I've always struggled to keep straight.

@justinvp
Copy link
Member

Note that we do look at the type hints at runtime and it was necessary to pass a definition for T to get_type_hints for it to work.

# Additionally, include a value for "T", which is needed for Input and InputType.
# We use the NoneType for the value of "T" because using TypeVar here doesn't work on Python 3.6
# and it doesn't matter what the value is for our purposes.
localns = {"Output": Output, "T": type(None)} # type: ignore

I think this was only needed for Input[T] and InputType[T] and was unrelated to Output[T]. The existing tests should cover us here, but I'm curious to see how running the examples with this change fairs. We may end up having to add a definition for T_co in the same way.

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.

Consider making Output[T] covariant in python mypy
7 participants