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

Revert sum literal integer change #13961

Merged
merged 1 commit into from Oct 29, 2022
Merged

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Oct 29, 2022

This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing to undo. Patching this in typeshed also feels weird, since there's a more general soundness issue. If a typevar has a bound or constraint, we might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)

This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird. If a typevar has a
bound or constraint, we might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.
@hauntsaninja
Copy link
Collaborator Author

cc @AlexWaygood , in case you have opinions

@AlexWaygood
Copy link
Member

cc @AlexWaygood , in case you have opinions

my opinion is that huge performance regressions are bad, feel free to go ahead 👍 :)

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Oct 29, 2022

Lol, alleged performance regressions though :-) I meant more on the upstreaming the revert question

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Oct 29, 2022

I also filed microsoft/pyright#4108 , so we'll see if Eric has a point of view

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hauntsaninja hauntsaninja merged commit 5319fa3 into python:master Oct 29, 2022
17 checks passed
@hauntsaninja hauntsaninja deleted the revert-sum branch Oct 29, 2022
hauntsaninja added a commit that referenced this pull request Oct 29, 2022
This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird, since there's a
more general soundness issue. If a typevar has a bound or constraint, we
might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)
hauntsaninja added a commit that referenced this pull request Oct 29, 2022
@AlexWaygood
Copy link
Member

I meant more on the upstreaming the revert question

Hah, I thought this was a PR to the typeshed repo when I posted my comment :)

The stub for sum has got quite a lot more complicated than it used to be over the course of many PRs during the last ~6 months or so. Some of those PRs were by me, some were by other people. In the end it seems like it's a surprisingly difficult function to write an accurate stub for, so maybe we'd be better off now if we hadn't had any of those sum PRs. There's still a few open typeshed issues about sum that I don't really know how to fix, and that have been caused by some of the recent increases in complexity.

I filed the PR adding _LiteralInteger to one of the sum overloads in response to a user complaint, so if we revert that PR over at typeshed, we should probably reopen that issue as once-again-unsolved.

If the sum change did cause a massive performance regression, I'd be curious to know whether pyright has Literal algorithms with the same bad time complexity, and whether it's therefore facing similar performance issues on codebases that have heavy use of sum. A performance regression on that scale is obviously completely unacceptable, so if it's happening to all type checkers, it definitely needs to be reverted over at typeshed.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Oct 29, 2022

Yeah, I guess in my mind there are two slightly different things at play here:

  1. The (alleged) performance issue. I'm assuming this is mypy specific. No one's provided a good minimal repro and I haven't had time to investigate, so there's actually not too much to go on. Hence my default would be to not make a change to typeshed for a mypy perf issue (unless e.g. some other typeshed demographic has a problem with it).

  2. The underlying soundness issue. This could conceivably be a thing fixed in mypy and pyright (although Eric closed as a won't fix for now). Although as I'm writing this, I realise that the solution I had in mind wouldn't work.

I'm currently thinking if I were to make a change to typeshed it might be pragmatic to change Iterable[bool | _LiteralInteger] to Iterable[bool | Literal[0, 1]]. This has the following advantages:

  • Still fixes the original typeshed report / the way this most likely comes up in real world code
  • Removes a weird soundness cliff at 26
  • I anticipate that if we are able to repro the perf issue, that reducing the size of the union would fix. There are some places where large unions make mypy slow

@AlexWaygood
Copy link
Member

I'm currently thinking if I were to make a change to typeshed it might be pragmatic to change Iterable[bool | _LiteralInteger] to Iterable[bool | Literal[0, 1]].

I'd be fine with that. The only reason I went with _LiteralInteger was because we already had the _PositiveInteger and _NegativeInteger aliases in builtins.pyi (so it was easy to do), and (forgetting that mypy has performance issues with large Literals), it seemed good to make the Literal as large as possible in order to avoid false positives.

  • Removes a weird soundness cliff at 26

Though there's a certain symmetry here with pow(), which has an awkward "oops, now we infer everything as Any" cliff at 26 :))

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 30, 2022

I agree that the underlying soundness issue is a thing, and is a pain. I think it's come up a few other times as well (but Eric's also right that it's not a very common issue). No idea how we could fix it, though — we'd probably need to somehow formalise exactly when it's unsound to solve a TypeVar with a Literal. Eric's probably right that this is maybe best discussed over at python/typing.

github-actions bot pushed a commit that referenced this pull request Nov 1, 2022
This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird, since there's a
more general soundness issue. If a typevar has a bound or constraint, we
might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)
github-actions bot pushed a commit that referenced this pull request Nov 2, 2022
This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird, since there's a
more general soundness issue. If a typevar has a bound or constraint, we
might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)
github-actions bot pushed a commit that referenced this pull request Nov 15, 2022
This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird, since there's a
more general soundness issue. If a typevar has a bound or constraint, we
might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)
github-actions bot pushed a commit that referenced this pull request Dec 1, 2022
This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird, since there's a
more general soundness issue. If a typevar has a bound or constraint, we
might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)
github-actions bot pushed a commit that referenced this pull request Dec 15, 2022
This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird, since there's a
more general soundness issue. If a typevar has a bound or constraint, we
might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)
github-actions bot pushed a commit that referenced this pull request Jan 1, 2023
This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird, since there's a
more general soundness issue. If a typevar has a bound or constraint, we
might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)
github-actions bot pushed a commit that referenced this pull request Jan 15, 2023
This is allegedly causing large performance problems, see 13821

typeshed/8231 had zero hits on mypy_primer, so it's not the worst thing
to undo. Patching this in typeshed also feels weird, since there's a
more general soundness issue. If a typevar has a bound or constraint, we
might not want to solve it to a Literal.

If we can confirm the performance regression or fix the unsoundness
within mypy, I might pursue upstreaming this in typeshed.

(Reminder: add this to the sync_typeshed script once merged)
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

2 participants