Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

New type inference: complete transitive closure#15754

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@ilevkivskyi
Copy link
Member

@ilevkivskyiilevkivskyi commentedJul 23, 2023
edited
Loading

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 onmypy_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 inFoundations 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 fortransitive_closure() which is the core of new logic.
  • While working on this I fixed couple bugs exposed inTypeVarTuple 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 addingextra_tvars as an attribute ofConstraint 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 ofinfer_constrains() and all related functions, which would be a really big refactoring.

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

@ilevkivskyi
Copy link
MemberAuthor

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
MemberAuthor

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

  • A bunch of changes inExpression: these are caused by extensive use ofParamSpec andConcatenate, which are not yet supported (and are planned in fourth PR)
  • A bunch of overlapping overload errors are caused by exposing an unrelated bug indetach_callable() (which is arguably a hack btw). It uses some heuristic to figure out what are the current class type variables instead of just callingself.scope.active_class(), and this causes it to create some bizarre functions likedef [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
MemberAuthor

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

@github-actions
Copy link
Contributor

Diff frommypy_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
MemberAuthor

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.

@ilevkivskyiilevkivskyi merged commit0d708cb intopython:masterAug 3, 2023
@ilevkivskyiilevkivskyi deleted the fix-generic-inference-2 branchAugust 3, 2023 23:17
ilevkivskyi added a commit that referenced this pull requestAug 9, 2023
This is a third PR in series following#15287 and#15754. This one is quite simple: Ijust add basic support for polymorphic inference involving typevariables with upper bounds and values. A complete support would bequite complicated, and it will be a corner case to already raresituation. Finally, it is written in a way that is easy to tune in thefuture.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 unittests as well).
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jhancejhanceAwaiting requested review from jhance

@JukkaLJukkaLAwaiting requested review from JukkaL

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ilevkivskyi

[8]ページ先頭

©2009-2025 Movatter.jp