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

feat: run hint tactics in parallel #8435

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

Conversation

semorrison
Copy link
Contributor

@semorrison semorrison commented Nov 16, 2023

Note that this is more resource intensive, as all hint tactics will start running at once.

I've worked out how to thread through cancellation hooks so as soon as one succeeds, any still running are cancelled. (If they are doing kernel computations or IO they might not listen, however!)


I've had to make this depend on #8635 to avoid breaking an assert_not_exists statement.

Open in Gitpod

test/hint.lean Outdated Show resolved Hide resolved
Comment on lines -28 to +40
• linarith
• exact lt_asymm h
• intro
• simp_all only [not_lt]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here for example we used to run linarith first, and as it succeeded that was all that was reported.

Now exact? succeeds more quickly, and that prevents linarith being reported.

Copy link
Collaborator

@thorimur thorimur left a comment

Choose a reason for hiding this comment

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

I've put the parallelism through some paces on my end, making sure that it e.g. doesn't propagate state between tasks, does in fact start all tasks before cancelling, leaves the outer monad in an appropriate state after takeUpToFirst and asArray, etc.... It behaved as expected in all cases! :)

These review comments are mostly very small and quick style changes for readability (and therefore maintainability). I tried to get rid of .2.1 style access in favor of clearer destructuring.

There's one meaningful comment in the test file, which is on how aesop produces a warning in one case which isn't reflected in the suggestions.

I did eventually figure out how to reduce all of the MLList.Meta code via MonadControl, but it preferably requires a core change because the instance is a bit wonky. We can leave that for another PR! :)

Mathlib/Data/MLList/IO.lean Outdated Show resolved Hide resolved
Mathlib/Data/MLList/Meta.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Hint.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Hint.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Hint.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Hint.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Hint.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Hint.lean Outdated Show resolved Hide resolved
test/hint.lean Show resolved Hide resolved
@semorrison semorrison added the blocked-by-other-PR This PR depends on another PR to Mathlib label Nov 27, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the blocked-by-other-PR This PR depends on another PR to Mathlib label Dec 11, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

This PR/issue depends on:

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Dec 14, 2023
@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Dec 14, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review The author would like community review of the PR merge-conflict The PR has a merge conflict with master, and needs manual merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants