Skip to content

Improvements to IOSim compatibility #12

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 10 commits into from
Sep 5, 2022

Conversation

MaximilianAlgehed
Copy link
Collaborator

This PR significantly simplifies the IOSim compatibility module by re-organizing StateModel to differentiate between the "model type" and the "perform type". One big plus of this is that we've won the ability to have models with different backends where the types might even by symbolic! (One can always hope!)

@MaximilianAlgehed
Copy link
Collaborator Author

@abailly Have a look at this - I think it's pretty cool how this works out!

@MaximilianAlgehed
Copy link
Collaborator Author

And yes, this is a major breaking change and we need to discuss how to best handle that. I also haven't finished everthing, but I think it's good getting a bit of a spotlight on this.

@MaximilianAlgehed MaximilianAlgehed requested a review from a user September 1, 2022 14:31
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Could you elaborate on the reason behind those changes? I am a bit confused because we already use the StateModel in hydra along with IOSim and it works fine. Perhaps we are doing things in a naive way?

@MaximilianAlgehed
Copy link
Collaborator Author

Could you elaborate on the reason behind those changes? I am a bit confused because we already use the StateModel in hydra along with IOSim and it works fine. Perhaps we are doing things in a naive way?

Have a look at the changes to the registry example. To be able to write properties where the state isn't indexed by s (which you need to be able to work properly with DL properties) you need the state type not to mention the s parameter.

@MaximilianAlgehed MaximilianAlgehed force-pushed the PR-iosim-compat-improvements branch 2 times, most recently from 568b51e to 7b4250d Compare September 2, 2022 06:52
and implement improved compatibility layer for IOSim in StateModel.
@MaximilianAlgehed MaximilianAlgehed force-pushed the PR-iosim-compat-improvements branch from 7b4250d to 31eca33 Compare September 2, 2022 06:53
@MaximilianAlgehed
Copy link
Collaborator Author

@abailly-iohk do you know why the build isn't working?

@ghost
Copy link

ghost commented Sep 2, 2022

It seems zlib-dev is missing but I don't understand why it fails now and wasn't before, you did not change anything in the dependencies. Adding mtl should not entail recompilation of the whole world. Don't bother with this, I can take care of turning it green later, once you're happy with the code and it builds locally.

@MaximilianAlgehed
Copy link
Collaborator Author

I think I'm pretty happy with the code now. I'll just bump the version and we can merge this.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I am still not entirely convinced we need the Realized typeclass to be part of the library: The Model should be completely independent of whatever Monad instance the RunModel is expressed in, and it should the latter's role to provide a translation layer which is specific to this execution engine. But I feel you know better so I will defer to your expertise.
Could you please update the CHANGELOG.md with some high-level statement about this change and also format the code with fourmolu for consistency's sake?

@MaximilianAlgehed
Copy link
Collaborator Author

@abailly-iohk I understand the concern about the Realized type family and splitting the postcondition out. However, I think the main arugments for this change are:

  1. There is already a level of indirection in the library (Var) and you don't want to force every new perform to introduce its own indirection tricks to deal with this situation.
  2. You want to make it possible to have many different runners - e.g. ST, IO, and IOSim or a concolic / symbolic testing engine and splitting out the postcondition allows you to have something like a symbolic postcondition that does something like SMT magic or an IO postcondition that queries the actual state of the system.

@MaximilianAlgehed
Copy link
Collaborator Author

Also the model is now completely independent of the monad that RunModel is expressed in!

What we could add back is a model postcondition. But since this would have to talk about the ideal return type of an action it's not clear what this would mean. It's certainly not clear that you can always obtain a value of the ideal return type (e.g. in a symbolic or IOSim runner).

@ghost
Copy link

ghost commented Sep 5, 2022

@MaximilianAlgehed
Copy link
Collaborator Author

Interestingly, we now have a failing test: https://github.com/input-output-hk/quickcheck-dynamic/runs/8183759215?check_suite_focus=true#step:7:36 🎉 🤔

Yes! That's on purpose ;) We could disable that test for now and I'll submit a new PR that turns it on. A fix to this failing test will hopefully be merged in io-sim soon:
input-output-hk/io-sim#19

@ghost
Copy link

ghost commented Sep 5, 2022

Understood. But then we can expect the failure so that it now passes and when iosim gets updated, it will fail again?

@MaximilianAlgehed
Copy link
Collaborator Author

When io-sim is updated we need to change the implementation of the Registry example a bit because io-sim is currently lacking threadStatus and we need it to correctly implement the example. So nothing will be "automatic" as it were. Easiest solution here is just commenting it out and as soon as the other PR is merged in io-sim I make the necessary changes to the example and add in the tests.

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

Unit Test Results

0 tests   - 1   0 ✔️  - 1   0s ⏱️ ±0s
0 suites  - 1   0 💤 ±0 
1 files   ±0   0 ±0 

Results for commit 7e3b5b6. ± Comparison against base commit f2c8c85.

♻️ This comment has been updated with latest results.

@ghost ghost self-requested a review September 5, 2022 08:44
@MaximilianAlgehed
Copy link
Collaborator Author

Waiting for it to turn green and then I'll merge and get started on the fourmolu PR.

@MaximilianAlgehed MaximilianAlgehed merged commit d17296e into main Sep 5, 2022
@MaximilianAlgehed MaximilianAlgehed deleted the PR-iosim-compat-improvements branch September 5, 2022 08:56
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.

2 participants