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

New type inference: complete transitive closure #15754

Merged
merged 16 commits into from
Aug 3, 2023

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented Jul 23, 2023

This is a first follow-up for #15287 (I like how my PR titles sound like research paper titles, LOL)

This PR completes the new type inference foundations by switching to a complete and well founded algorithm [1] for transitive closure (that replaces more ad hoc initial algorithm that covered 80% of cases and was good for experimenting with new inference scheme). In particular the algorithm in this PR covers two important edge cases (see tests). Some comments:

  • I don't intend to switch the default for --new-type-inference, I just want to see the effect of the switch on mypy_primer, I will switch back to false before merging
  • This flag is still not ready to be publicly announced, I am going to make another 2-3 PRs from the list in Foundations for non-linear solver and polymorphic application #15287 before making this public.
  • I am not adding yet the unit tests as discussed in previous PR. This PR is already quite big, and the next one (support for upper bounds and values) should be much smaller. I am going to add unit tests only for transitive_closure() which is the core of new logic.
  • While working on this I fixed couple bugs exposed in TypeVarTuple support: one is rare technical corner case, another one is serious, template and actual where swapped during constraint inference, effectively causing outer/return context to be completely ignored for instances.
  • It is better to review the PR with "ignore whitespace" option turned on (there is big chunk in solve.py that is just change of indentation).
  • There is one questionable design choice I am making in this PR, I am adding extra_tvars as an attribute of Constraint class, while it logically should not be attributed to any individual constraint, but rather to the full list of constrains. However, doing this properly would require changing the return type of infer_constrains() and all related functions, which would be a really big refactoring.

[1] Definition 7.1 in https://inria.hal.science/inria-00073205/document

@ilevkivskyi
Copy link
Member Author

Btw I guess I should quote the thesis where I took the algorithm I am using here. I will update the PR description.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

OK, I went through mypy_primer and I am generally happy, most of the output matches the previous PR. There are two differences:

  • A bunch of changes in Expression: these are caused by extensive use of ParamSpec and Concatenate, which are not yet supported (and are planned in fourth PR)
  • A bunch of overlapping overload errors are caused by exposing an unrelated bug in detach_callable() (which is arguably a hack btw). It uses some heuristic to figure out what are the current class type variables instead of just calling self.scope.active_class(), and this causes it to create some bizarre functions like def [T] () -> def [T] (T) -> T which accidentally used to work before. I am going to push a fix for this now.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

All looks good, I will now switch the default back to false to prepare for merge.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
- src/hydra_zen/typing/_builds_overloads.py:44: error: Overloaded function signatures 1 and 2 overlap with incompatible return types  [misc]
- src/hydra_zen/typing/_builds_overloads.py:44: error: Overloaded function signatures 1 and 9 overlap with incompatible return types  [misc]
- src/hydra_zen/typing/_builds_overloads.py:270: error: Overloaded function signatures 1 and 3 overlap with incompatible return types  [misc]
- src/hydra_zen/typing/_builds_overloads.py:270: error: Overloaded function signatures 1 and 10 overlap with incompatible return types  [misc]
- src/hydra_zen/typing/_builds_overloads.py:561: error: Overloaded function signatures 3 and 4 overlap with incompatible return types  [misc]
- src/hydra_zen/typing/_builds_overloads.py:561: error: Overloaded function signatures 3 and 7 overlap with incompatible return types  [misc]
- src/hydra_zen/structured_configs/_implementations.py:834: error: Overloaded function signatures 1 and 2 overlap with incompatible return types  [misc]
- src/hydra_zen/structured_configs/_implementations.py:834: error: Overloaded function signatures 1 and 9 overlap with incompatible return types  [misc]

@ilevkivskyi
Copy link
Member Author

OK, it looks like no one is around to take a look at this. I have the next PR ready, so I am going to merge this tomorrow, unless there are objections.

@ilevkivskyi ilevkivskyi merged commit 0d708cb into python:master Aug 3, 2023
@ilevkivskyi ilevkivskyi deleted the fix-generic-inference-2 branch August 3, 2023 23:17
ilevkivskyi added a commit that referenced this pull request Aug 9, 2023
This is a third PR in series following
#15287 and
#15754. This one is quite simple: I
just add basic support for polymorphic inference involving type
variables with upper bounds and values. A complete support would be
quite complicated, and it will be a corner case to already rare
situation. Finally, it is written in a way that is easy to tune in the
future.

I also use this PR to add some unit tests for all three PRs so far,
other two PRs only added integration tests (and I clean up existing unit
tests as well).
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.

1 participant