Skip to content

connect maxHeartbeats in CoreM to kernel variable max_heartbeats#1364

Closed
bollu wants to merge 7 commits intoleanprover:masterfrom
bollu:2022-jul-20-unify-heartbeat
Closed

connect maxHeartbeats in CoreM to kernel variable max_heartbeats#1364
bollu wants to merge 7 commits intoleanprover:masterfrom
bollu:2022-jul-20-unify-heartbeat

Conversation

@bollu
Copy link
Copy Markdown
Contributor

@bollu bollu commented Jul 22, 2022

No description provided.

@github-actions
Copy link
Copy Markdown
Contributor

Thanks for your contribution! Please make sure to follow our Commit Convention.

@bollu bollu marked this pull request as draft July 22, 2022 19:28
@Kha
Copy link
Copy Markdown
Member

Kha commented Jul 25, 2022

The basic approach looks good (assuming it works :) ). Is there anything left to do apart from cleaning up the PR?

def addAndCompile [Monad m] [MonadEnv m] [MonadError m] [MonadOptions m] [MonadLog m] [AddMessageContext m] (decl : Declaration) : m Unit := do
addDecl decl;
compileDecl decl
def addAndCompile [Monad m] [MonadEnv m] [MonadError m] [MonadOptions m] [MonadLog m] [AddMessageContext m] (maxHeartbeats: Nat) (decl : Declaration) : m Unit := do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def addAndCompile [Monad m] [MonadEnv m] [MonadError m] [MonadOptions m] [MonadLog m] [AddMessageContext m] (maxHeartbeats: Nat) (decl : Declaration) : m Unit := do
def addAndCompile (decl : Declaration) : CoreM Unit := do

I think we should avoid the extra argument here. (And adding a MonadHeartbeat class feels overkill here, this isn't even a combinator.)

structure Context where
maxResultSize : Nat
maxHeartbeats : Nat
maxHeartbeats : Nat := 6543
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please revert.

@bollu
Copy link
Copy Markdown
Contributor Author

bollu commented Jul 28, 2022

I didn't run the test suite while I was at the hackathon since my laptop is quite small :) On coming back home and running the test suite, I find a large number of tests break:

https://gist.github.com/bollu/f8608a2849bde346c64d7f6ebb10ca71

I'll be debugging this tomorrow, but if anyone has any quick guesses as to why this might be the case, I'd appreciate it!

@Kha
Copy link
Copy Markdown
Member

Kha commented Jul 28, 2022

For "quick guesses", --output-on-failure would have been helpful :) . And the CI won't run until you address the merge conflicts.

@leodemoura leodemoura added the awaiting-author Waiting for PR author to address issues label Jul 28, 2022
@leodemoura
Copy link
Copy Markdown
Member

@bollu Are you still planning to work on this PR, or should I close it?

@bollu
Copy link
Copy Markdown
Contributor Author

bollu commented Sep 5, 2022

@leodemoura Closed to clear Lean's PR queue; I shall send a completed PR once the solution does not fail on seemingly unrelated test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-author Waiting for PR author to address issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants