Skip to content

Alternative approach to Annotator #4733

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.

Sign up for GitHub

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

Jump to bottom
Merged
lehins merged 2 commits into from
Nov 2, 2024
Merged

Alternative approach to Annotator #4733

lehins merged 2 commits into from
Nov 2, 2024

Conversation

@lehins
Copy link
Collaborator

@lehins lehins commented Oct 30, 2024 •
edited
Loading

Description

Add Maybe ByteString to versioned Decoder. This approach allows us to get access to the original bytes inside of any decoder.

Fixes #4009

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • 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)
  • 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)
  • 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

@lehins lehins changed the title Alternative Annotator approach works Alternative approach to Annotator Oct 30, 2024
@lehins lehins force-pushed the lehins/alternative-to-annotator branch 2 times, most recently from 2684b58 to dcec2dc Compare October 30, 2024 14:46
@lehins lehins marked this pull request as ready for review October 30, 2024 14:57
@lehins lehins requested a review from a team as a code owner October 30, 2024 14:57
@lehins lehins force-pushed the lehins/alternative-to-annotator branch from dcec2dc to c82b437 Compare October 31, 2024 20:25
Lucsanszky
Lucsanszky reviewed Nov 1, 2024
View reviewed changes
Copy link
Contributor

@Lucsanszky Lucsanszky left a comment

Choose a reason for hiding this comment

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

This is just a partial review, as I plan to take a better look.

Lucsanszky
Lucsanszky approved these changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@Lucsanszky Lucsanszky left a comment

Choose a reason for hiding this comment

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

Had a better look, LGTM! 👍

One question: you mention that the problem with Annotator is the fact that it's an Applicative and thus we can't look into the item that's being decoded. Making Annotator monadic might solve this problem but that brings us to the already existing performance issue, which could potentially get worse with a monadic formulation for Annotator but who knows, it might save us from constructing that big closure as well. I don't know, I haven't tried implementing it, so I'm just bouncing ideas around. I wouldn't be surprised if you have already tried and ruled this approach out.

@lehins
Copy link
Collaborator Author

lehins commented Nov 1, 2024

Making Annotator monadic might solve this problem

That would not help. It would have to be a monad transformer with Decoder being the base monad. That is because we need to inspect contents in the Decoder monad, not inside the Annotator.

@lehins
Copy link
Collaborator Author

lehins commented Nov 1, 2024

In any case. Everyone hates Annotator, including myself. So, I am looking forward to the day when we completely dump it in the garbage 😄

lehins and others added 2 commits November 1, 2024 16:24
@lehins
Add Maybe ByteString to Decoder
425fe83 
Alternative Annotator approach works
@lehins @Lucsanszky
Update libs/cardano-ledger-binary/CHANGELOG.md
99683b3 
Co-authored-by: Lucsanszky <[email protected]>
@lehins lehins force-pushed the lehins/alternative-to-annotator branch from 8de82f5 to 99683b3 Compare November 1, 2024 22:24
@lehins lehins enabled auto-merge (squash) November 1, 2024 22:25
@lehins lehins merged commit 97c2c87 into master Nov 2, 2024
154 checks passed
@lehins lehins deleted the lehins/alternative-to-annotator branch November 2, 2024 00:33
@lehins lehins mentioned this pull request Jan 6, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lucsanszky Lucsanszky Lucsanszky approved these changes

@aniketd aniketd Awaiting requested review from aniketd

@TimSheard TimSheard Awaiting requested review from TimSheard

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Invent a better way to retain bytes due to limitations of Annotator

3 participants

@lehins @Lucsanszky