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

Add tracing utilities; with cabal flags switching #39

Merged
merged 26 commits into from
Jan 5, 2022
Merged

Add tracing utilities; with cabal flags switching #39

merged 26 commits into from
Jan 5, 2022

Conversation

TotallyNotChase
Copy link
Collaborator

It's added as a separate package - plutarch-trace. Everything one needs to know to use plutarch-trace and switch between tracing/no-tracing is explained in plutarch-trace/README. Users can also have both tracing and no-tracing version at the same time.

Cabal >= 3.0 will be required to add plutarch-trace as a dependency.

Some general notes-

  • Any testing of these new utilities should be done within plutarch-tracing itself, after instantiating a signature. This is because plutarch-tracing depends on plutarch.

  • plutarch-tracing exposes a generic module. That is, a module that actually uses a signature under the hood. In fact, the module just re-exports the signature.

    Why not just have the signature? You cannot instantiate a signature directly and get to using it. You need to have a package with a module that uses said signature, then you need to instantiate that package using an implementation. You cannot do-

    plutarch-trace (Plutarch.TraceSig) requires (Plutarch.TraceSig as Plutarch.Trace.Enable),
    

    where Plutarch.TraceSig is a signature.

    TL;DR - In our case, dev experience will be better with this structure. No one should ever have to import Plutarch.TraceSig.

  • Cabal will give off warnings about using experimental features and some things are clearly a bit finicky. But it's all good, this usecase is not too complex.

@L-as
Copy link
Member

L-as commented Dec 28, 2021

Question: Is it necessary to have a separate library plutarch-trace?

@TotallyNotChase
Copy link
Collaborator Author

Yes. So the thing about mixins is that you have to explicitly name every single module you want to import in mixins. If it was part of Plutarch, for example, you can't do-

plutarch (Plutarch.Trace) requires (Plutarch.TraceSig as Plutarch.Trace.Enable),

It would only make Plutarch.Trace visible. You need to list all the modules in there.

I think this is why signature packages are generally tiny and don't export many modules.

@L-as
Copy link
Member

L-as commented Dec 28, 2021

Makes sense. As for the tests, we this should be fine. We should be able to depend on plutarch-trace in the tests.

@TotallyNotChase
Copy link
Collaborator Author

I wonder if plutarch requires (Plutarch.TraceSig as Plutarch.Trace.Enable) works. I was under the impression it doesn't from really-small-backpack-example.

Also, I think you'll get a cyclic dependency error if you try adding plutarch-trace to the current test suite. Pretty sure I tried that. Might need to make the test suite in another package all together.

Copy link
Member

@L-as L-as left a comment

Choose a reason for hiding this comment

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

Please remove the tracing utilities plutarch and add some tests.

plutarch-trace/impl/Plutarch/Trace/Disable.hs Outdated Show resolved Hide resolved
plutarch-trace/src/Plutarch/TraceSig.hsig Outdated Show resolved Hide resolved
plutarch-trace/README.md Outdated Show resolved Hide resolved
@TotallyNotChase
Copy link
Collaborator Author

Please remove the tracing utilities plutarch and add some tests.

Sorry, didn't get what you mean here. Remove what?

@L-as
Copy link
Member

L-as commented Dec 28, 2021

haskell/cabal#3422

We probably want to add a separate package for tests.

@TotallyNotChase TotallyNotChase linked an issue Dec 28, 2021 that may be closed by this pull request
@L-as
Copy link
Member

L-as commented Dec 28, 2021

Please remove the tracing utilities plutarch and add some tests.

Sorry, didn't get what you mean here. Remove what?

This is entirely my mistake. I was thinking of the code you wrote outside of this repository for tracing and had forgotten that that wasn't in Plutarch yet.

@L-as
Copy link
Member

L-as commented Dec 28, 2021

Relevant: IntersectMBO/plutus#4304

@L-as
Copy link
Member

L-as commented Dec 28, 2021

I'm honestly still not sure whether Backpack is the best choice, but since I have very little experience with it, I'm thinking it's worth trying. Worst case we rip it out and make a backward incompatible change.

@TotallyNotChase
Copy link
Collaborator Author

TotallyNotChase commented Dec 28, 2021

I'm not super sure about backpack either ¯\(ツ)

But from what I've seen so far - I don't think we'll need to rip it out. I've learnt from Drazen that cabal flags aren't exactly the best for even slightly complex dev UX. I like the ability to have both versions at once. I like the easy switching. I like the painless propagation of "generic-ness". And for our simple usecase, this shouldn't cause any problems. We won't really be mending much with the tracing package anyway.

@TotallyNotChase
Copy link
Collaborator Author

By the way, it looks like plutarch requires (Plutarch.TraceSig as Plutarch.Trace.Enable) works for importing all modules! You can't use the renaming feature at all if you do that implicit import all though. So we still definitely want plutarch-tracing as a separate package.

@Fresheyeball
Copy link

@L-as @TotallyNotChase I would be surprised if backpack gets ongoing support in GHC. It's very unpopular, and the economics are terrible. It's not really a first module system for Haskell a la Standard ML, so much as a hacked on module system. I would avoid backpack if possible. I have some alternative approaches if you want.

@TotallyNotChase
Copy link
Collaborator Author

Would love to hear about alternative approaches! Not a fan of cabal flags + CPP either.

@L-as
Copy link
Member

L-as commented Dec 28, 2021

I had some discussions on #haskell:libera.chat wrt. this. Seems like nobody is maintaining Backpack, not to mention it's still unsupported in Stack. The implementation in GHC is supposedly not even finished. I'm not sure what the correct decision to take here is. Considering it's a small part of Plutarch, we can always switch it out if needed.

@L-as
Copy link
Member

L-as commented Dec 28, 2021

@TotallyNotChase
Copy link
Collaborator Author

I've noticed Matthew Pickering has started to make an effort at improving backpack 2 months ago. That's pretty neat!

I find it funny that the backpack unofficial documentation, through the examples, is extremely good. Getting up and running with backpack is super easy with really-small-backpack-example. But the official documentation is non-existent apart from cabal.

Also, I'm totally fine with switching the backpack impl out for some other machinery. I had fun learning it anyway. Although I do personally like backpack, we don't have certainty of its future.

@L-as
Copy link
Member

L-as commented Dec 29, 2021

I think we should go with the Backpack approach for now. Doesn't make sense timewise to rewrite it now in case support gets dropped in the future. We can always handle this then.

@TotallyNotChase
Copy link
Collaborator Author

@L-as Moved out the tests suite to a separate package and added tests for tracing/no-tracing. I'm not sure if the CI ran them though.

Otherwise haskell.nix won't detect it
It is more efficient, albeit if you care about efficiency
you'd just disable traces entirely, and `ptrace'` can't be
elided completely, unlike `ptrace`.
@L-as
Copy link
Member

L-as commented Dec 30, 2021

It seems like we'll have to move off of Backpack...

The only other clean solution I can think of is passing in ptrace using implicit arguments / type classes, but that would mean every function now needs to do Something => Term s ..., which would be a huge breaking change.

The alternative is that we fix support for Backpack but from what I can tell, this is far from trivial.
It's not even support by the Haskell infrastructure in Nixpkgs.

We'll probably have to use flags and CPP unfortunately.

@blamario
Copy link
Collaborator

blamario commented Jan 3, 2022

I suppose I'm missing something, but why do we need to elevate this to the level of Cabal? Why wouldn't something as simple as https://hackage.haskell.org/package/NoTrace work well enough?

@L-as
Copy link
Member

L-as commented Jan 3, 2022

Is there a good programmatic way of switching automatically to that? I certainly don't think we need Backpack, but just adding add Plutarch.NoTrace doesn't solve it.

@TotallyNotChase
Copy link
Collaborator Author

@L-as Ha, I expected something of this sort to happen. Let's just get this over with - switched to cabal flags + CPP.

I think this also means the CI should re-run the "Tracing" testcase with a -f development now. development flag is off by default.

@L-as
Copy link
Member

L-as commented Jan 4, 2022

@TotallyNotChase We don't need to use CPP. We can use the flags in the cabal file to select between different source files.
Although I'm not sure whether this is a better approach.

@TotallyNotChase
Copy link
Collaborator Author

@TotallyNotChase We don't need to use CPP. We can use the flags in the cabal file to select between different source files. Although I'm not sure whether this is a better approach.

CPP reduces some code duplication compared to that approach. I don't think it really matters though.

@L-as
Copy link
Member

L-as commented Jan 4, 2022

Is this missing anything? I looked at the code and it seemed fine to me.

@TotallyNotChase
Copy link
Collaborator Author

@L-as It should be ready to merge.

@TotallyNotChase TotallyNotChase changed the title Add tracing utilities; with easy switching using backpack Add tracing utilities; with cabal flags switching Jan 5, 2022
Copy link
Member

@L-as L-as left a comment

Choose a reason for hiding this comment

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

LGTM

@L-as L-as merged commit 99ef74f into master Jan 5, 2022
@TotallyNotChase TotallyNotChase deleted the tracing branch January 5, 2022 10:22
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.

Add tracing utilities
4 participants