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

Constrained v2 #3895

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Constrained v2 #3895

merged 4 commits into from
Feb 2, 2024

Conversation

MaximilianAlgehed
Copy link
Collaborator

Description

This PR introduces version 2 of the Constrained approach to generating random data.

It's a significant overhaul of the existing approach and it's far from done. However, it is at the point where
it can be useful for writing properties (e.g. for the GOV rule) so getting at least a WIP PR up seems very valuable.

Furthermore, if we can get this merged we might be able to start working on some of the STS property issues in parallel with improving this.

@lehins any guidance on setting up the separate package would be much appreciated - is there a README somewhere I should follow?

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@MaximilianAlgehed MaximilianAlgehed linked an issue Nov 29, 2023 that may be closed by this pull request
@MaximilianAlgehed MaximilianAlgehed marked this pull request as draft November 29, 2023 10:06
Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

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

There is a lot of stuff in here. I'm not going to have time to do a proper review until next week.

libs/constrained/src/Constrained/Core.hs Outdated Show resolved Hide resolved
libs/constrained/src/Constrained/GenT.hs Outdated Show resolved Hide resolved
@lehins
Copy link
Contributor

lehins commented Nov 29, 2023

@lehins any guidance on setting up the separate package would be much appreciated

I think you got it.

@MaximilianAlgehed
Copy link
Collaborator Author

@lehins any guidance on setting up the separate package would be much appreciated

I think you got it.

No nix crap in other words 😅

@MaximilianAlgehed MaximilianAlgehed linked an issue Nov 29, 2023 that may be closed by this pull request
cabal.project Outdated Show resolved Hide resolved
@MaximilianAlgehed MaximilianAlgehed force-pushed the constrained-v2 branch 3 times, most recently from ec50741 to 1faca43 Compare December 5, 2023 14:31
@MaximilianAlgehed
Copy link
Collaborator Author

@lehins the formatter is broken again...

@lehins
Copy link
Contributor

lehins commented Dec 12, 2023

@MaximilianAlgehed, fourmolu is not yet on par with your advanced Haskell techniques? 😄

Why do you say it is broken. Looks like fourmolu on CI is passing.

@MaximilianAlgehed
Copy link
Collaborator Author

It's passing now because I changed the code to work around the bug the tool itself told me that it has a few commits ago:

Loaded config from: /home/runner/work/cardano-ledger/cardano-ledger/fourmolu.yaml
libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/V2/Conway/Gov.hs
@@ -179,7 +179,8 @@
           cppGovActionLifetime -- :: !(THKD 'GovGroup f EpochInterval)
           cppGovActionDeposit -- :: !(THKD 'GovGroup f Coin)
           cppDRepDeposit -- :: !(THKD 'GovGroup f Coin)
-          _cppDRepActivity -> -- :: !(THKD 'GovGroup f EpochInterval)
+          _cppDRepActivity ->
+             -- :: !(THKD 'GovGroup f EpochInterval)
              [ cppMaxBBSize /=. Lit (THKD $ SJust 0)
              , cppMaxTxSize /=. Lit (THKD $ SJust 0)
              , cppMaxBHSize /=. Lit (THKD $ SJust 0)

  Formatting is not idempotent.
  Please, consider reporting the bug.
libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/V2/Conway/PParams.hs
@@ -4[5](https://github.com/input-output-hk/cardano-ledger/actions/runs/7186214519/job/19571131809#step:4:6),[7](https://github.com/input-output-hk/cardano-ledger/actions/runs/7186214519/job/19571131809#step:4:8) +45,[8](https://github.com/input-output-hk/cardano-ledger/actions/runs/7186214519/job/19571131809#step:4:9) @@
           cppGovActionLifetime -- :: !(THKD 'GovGroup f EpochInterval)
           cppGovActionDeposit -- :: !(THKD 'GovGroup f Coin)
           cppDRepDeposit -- :: !(THKD 'GovGroup f Coin)
-          _cppDRepActivity -> -- :: !(THKD 'GovGroup f EpochInterval)
+          _cppDRepActivity ->
+             -- :: !(THKD 'GovGroup f EpochInterval)
              [ cppMaxBBSize /=. Lit (THKD 0)
              , cppMaxTxSize /=. Lit (THKD 0)
              , cppMaxBHSize /=. Lit (THKD 0)

  Formatting is not idempotent.
  Please, consider reporting the bug.

@lehins
Copy link
Contributor

lehins commented Dec 12, 2023

Looks like it chokes on some comments. I am not too worried about that, since comments can always be adjusted in a manner that makes fourmolu happy.

An occasional hiccup like that is a very small price to pay for consistently formatted code that is actually readable.

@MaximilianAlgehed
Copy link
Collaborator Author

@lehins in this case the comments needed to be where they were to make sense (they were pointing out the type of each argument in a lambda without too much noise. But anyway, the comments are not a huge deal.

Let's agree to disagree on the "actually readable" point.

@MaximilianAlgehed
Copy link
Collaborator Author

MaximilianAlgehed commented Dec 12, 2023

My main complaints with it are:

  1. It doesn't group arguments nicely, ruining code like:
    caseOn b
      (branch $ \ a -> ...)
      (branch $ \ b -> ...)
    
    by turning it into things like:
     caseOn
       b
       (branch $ \ a -> ...)
       (branch $ \ b -> ...)
    
    which is unnecessarily space-consuming and weird-looking.
  2. It doesn't align cases, meaning it turns stuff like:
    foo = case bar of
        Bar a b c d -> True
        Baz a       -> False
        Doodle c    -> True
    
    into stuff like
    foo = case bar of
        Bar a b c d -> True
        Baz a -> False
        Doodle c -> True
    
    which can get really cramped in big functions with lots of cases
  3. It doesn't align type signatures and has no option to put -> at the start of the line.

Anyway, I can deal with it but I would never turn it on myself.

@lehins
Copy link
Contributor

lehins commented Dec 12, 2023

Anyway, I can deal with it but I would never turn it on myself.

You would, if you were part of a big team of developers each with their own style of writing Haskell.

My main complaints with it are:

Every single one of those points slightly affects the aesthetics of the code, which I can certainly agree with. However, fourmolu is an enormous time saver:

  • We never have to manually adjust the code to "make it look pretty" or conform to some specific style
  • We never have to argue with team mates on what is more appropriate style of coding.

Let me tell you, I will rather deal with slightly quirky looking code than the alternatives of:

Oh yeah, and one more point, fourmolu style is very git diff friendly! Which is another huge time saver.

It doesn't align cases, meaning it turns stuff like:

Example with case alignment you pointed out is very git unfriendly.

It doesn't align type signatures and has no option to put -> at the start of the line.

It actually does, it is one of the configuration options which would apply throughout the codebase, which we choose not to enable.

@MaximilianAlgehed
Copy link
Collaborator Author

You would, if you were part of a big team of developers each with their own style of writing Haskell.

There was that one thing about assumptions and donkeys...

I absolutely understand the point of using formatters, but the main issue I find is that they make code difficult to read. Pf course, opinions can differ on what makes code readable, but my opinion is that fourmolu is particularly quirky...

But yes, the co-ordination problem is unfortunately particularily difficult to solve in Haskell - I dont know of any reasonable tools...

Every single one of those points slightly affects the aesthetics of the code, which I can certainly agree with.

The problem with all these tools is that the inevitable quirks compound to make the code difficult to read. Now, you can argue on weather or not its code or diffs that should be read - my philosophy is that readable code is much more important than readable diffs. But reasonable people can differ on this point.

@Soupstraw
Copy link
Contributor

1. It doesn't group arguments nicely, ruining code like:
   ```
   caseOn b
     (branch $ \ a -> ...)
     (branch $ \ b -> ...)
   ```

I think it should sort of work if you put parentheses around caseOn b.

@MaximilianAlgehed MaximilianAlgehed marked this pull request as ready for review December 14, 2023 18:57
@MaximilianAlgehed
Copy link
Collaborator Author

@lehins and @Soupstraw I've marked this ready for review - it would be great if you wanted to take a look. There is still quite a lot left to do in terms of documentation and fixing some remaining issues, but I'm at the point where I'm anyway doing a lot of cleanup and writing a bunch of documentation so having some inputs during the process wouldn't be a bad thing.

Copy link
Contributor

@TimSheard TimSheard left a comment

Choose a reason for hiding this comment

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

This looks great. Please address the conversations by improving the comment where indicated

libs/constrained/src/Constrained/Base.hs Outdated Show resolved Hide resolved
libs/constrained-generators/src/Constrained/List.hs Outdated Show resolved Hide resolved
libs/constrained-generators/src/Constrained/Test.hs Outdated Show resolved Hide resolved
libs/constrained-generators/src/Constrained/Univ.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@Soupstraw Soupstraw left a comment

Choose a reason for hiding this comment

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

LGTM, best get this merged soon so we can start experimenting with it

@MaximilianAlgehed MaximilianAlgehed force-pushed the constrained-v2 branch 3 times, most recently from 7c766d8 to 6c2d7e9 Compare January 18, 2024 10:56
@MaximilianAlgehed
Copy link
Collaborator Author

@lehins @Soupstraw and @TimSheard that's all the comments resolved for now, anything else just let me know. Now we have to try to get everything passing.

libs/cardano-ledger-test/cardano-ledger-test.cabal Outdated Show resolved Hide resolved
libs/cardano-ledger-test/cardano-ledger-test.cabal Outdated Show resolved Hide resolved
libs/cardano-ledger-test/cardano-ledger-test.cabal Outdated Show resolved Hide resolved
libs/constrained/src/Constrained/GenT.hs Outdated Show resolved Hide resolved
@UlfNorell UlfNorell force-pushed the constrained-v2 branch 6 times, most recently from 51665d2 to 9c58129 Compare January 27, 2024 11:19
@UlfNorell
Copy link
Collaborator

This passes Haskell CI now. I had to do some acrobatics to make it build with 8.10.7, but nothing too horrendous.

I have no idea what's up with the Hydra failures. It fails on nix things that I'm pretty sure we haven't messed with, but I could be wrong. If someone could take a look that would be much appreciated.

@UlfNorell
Copy link
Collaborator

@lehins Is the hydra failures anything to worry about or can we merge this?

@lehins
Copy link
Contributor

lehins commented Feb 2, 2024

Is the hydra failures anything to worry about or can we merge this?

Nope, nothing to worry about

@lehins lehins merged commit 2ccd4ae into master Feb 2, 2024
11 of 31 checks passed
@UlfNorell UlfNorell deleted the constrained-v2 branch February 2, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants