Skip to content

RPC trace cleanup: remove simplification logs #3880

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

Merged
merged 7 commits into from
May 21, 2024

Conversation

geo2a
Copy link
Contributor

@geo2a geo2a commented May 20, 2024

Fixes #3871

Note that removing the simplification log request parameters and the simplification log entries form the response is same, because pyk is already modified to not recognize them, see:

But even considering that, we should run pyk's integration tests before merging.

@geo2a geo2a force-pushed the georgy/rpc-trace-cleanup branch from 241b566 to 9fe027e Compare May 20, 2024 10:10
@geo2a geo2a force-pushed the georgy/rpc-trace-cleanup branch from 84d86aa to d93ebe4 Compare May 21, 2024 05:45
Copy link
Member

@jberthold jberthold left a comment

Choose a reason for hiding this comment

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

While we are at it, we could maybe clean up more - the RewriteSimplified constructor in RewriteTrace and all related code is one example.
(can also merge this and proceed with other things, though)

Comment on lines 18 to 19
EquationTrace (..),
pattern CollectEquationTraces,
pattern NoCollectEquationTraces,
eraseStates,
Copy link
Member

Choose a reason for hiding this comment

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

Am I right that we shouldn't export EquationTrace and eraseStates any more here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks, removed them

@geo2a
Copy link
Contributor Author

geo2a commented May 21, 2024

I ran the pyk integration tests and they pass.

@jberthold sounds good, I'll do a bit more clean-up.

@geo2a
Copy link
Contributor Author

geo2a commented May 21, 2024

@jberthold I did not fully remove the RewriteSimplified constructor as it is used for the -l Rewrite logging level as I understand.

I think it is probably enough clean-up at that point.

@jberthold
Copy link
Member

@jberthold I did not fully remove the RewriteSimplified constructor as it is used for the -l Rewrite logging level as I understand.

I think it is only used for error reporting but nvm 🤷

@geo2a
Copy link
Contributor Author

geo2a commented May 21, 2024

We probably need to update K once runtimeverification/k#4367 is merged to fix Kore integration tests:

error: hash mismatch in fixed-output derivation '/nix/store/0wsq2aqzqlcb0g61jm389zwqmmjjjsmz-k-7.0.40-dirty-maven-deps.drv':
         specified: sha256-DnY+Zx4fr3iuI24VjlnCVeQCcK6aHYYAEW13aUEzd+I=
            got:    sha256-6rGuU8NFSp3lQVxxPj5WREpNVPneMqPH45DFEmLiH6I=

Copy link
Contributor

@goodlyrottenapple goodlyrottenapple left a comment

Choose a reason for hiding this comment

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

Nice! Great to see the cleanup :)

@rv-jenkins rv-jenkins merged commit 0f31483 into master May 21, 2024
7 checks passed
@rv-jenkins rv-jenkins deleted the georgy/rpc-trace-cleanup branch May 21, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove code that collects and returns RPC simplification logs
4 participants