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

Experimental Bindgen for Chicory modules #506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andreaTP
Copy link
Collaborator

@andreaTP andreaTP commented Sep 2, 2024

This should be ready for review, I turned it to use #496 and simplified where possible.
Given the fact that the API of Chicory is not stable, the only reasonable place to develop bindgens is this very same repository, we can revisit this decision in the future.

Happy to hear feedback!
(note #510 should be merged first)

old text:

Opening in draft as I noticed that some work done here is partially overlapping with #496 .
Looking for early feedback, especially from @electrum and @evacchi .

For the reviewers, this test shows the expected user experience: https://github.com/dylibso/chicory/compare/main...andreaTP:bindgen?expand=1#diff-6ae6c6d43974944561891d7bfc45e572fca4e97c37547b393527d5ad08ba97f0R15-R70 it can be a little better, but it's in the right direction, wdyt?

Where to go next?
I can move this to the (upcoming)annotation processor or I can roll it out as a Maven plugin, in the latter case I think we should start to refactor some of the logic in a shared library.

Notes:

@andreaTP andreaTP changed the title [WIP] Chicory Bindgen Experimental Bindgen for Chicory modules Sep 3, 2024
@andreaTP andreaTP marked this pull request as ready for review September 3, 2024 13:58
// Act
var ctxAddr = opa.opaEvalCtxNew();
var input = "{\"user\": \"alice\"}";
var inputStrAddr = opa.opaMalloc(input.length());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@electrum I haven't checked yet, but I would verify #507 on this kind of examples.

*/
@Parameter(
required = true,
defaultValue = "${project.build.directory}/generated-sources/chciory-bindgen")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo "chciory"

@electrum
Copy link
Collaborator

electrum commented Sep 7, 2024

I now understand what you're doing here: generate a Java class containing the exports from a WASM file. This givens you a low-level C-like interface that directly matches the WASM interface. That's not bad, and it's useful, better than writing the code by hand.

What I have in mind is different: an idiomatic Java interface. This is similar to the annotation support for host modules. Doing this requires additional, higher level information about the WASM interface. The most direct way is to write the Java interface by hand, providing the additional information as annotations.

For a large WASM interface, it might be useful to have a code generator that you can run once to get you started, then you can edit it to clean things up. However, with GitHub Copilot or similar AI assistants, the generator might not be needed.

I'll use your OPA example to show what I'm envisioning here.

@andreaTP
Copy link
Collaborator Author

andreaTP commented Sep 9, 2024

This givens you a low-level C-like interface that directly matches the WASM interface. That's not bad, and it's useful, better than writing the code by hand.

This has been a big pain point(for me) when integrating multiple different modules.

For a large WASM interface, it might be useful to have a code generator that you can run once to get you started,

From my experience, I think that having something completely automatic able to verify at compile time the compatibility of the module API with what has been implemented in Java is instrumental to enable easy upgrades and maintainability.

I have been writing this "low level" code enough times now, and is super error-prone and boring to write and maintain.
I do not like to rescue to AI when we are in complete control of all the invariants and we can come up with something that already cover most use-cases without much code.

@bhelx
Copy link
Contributor

bhelx commented Sep 10, 2024

I'm still digesting this, but some thoughts on first look. I think this is a lower level concept than what I'd call bindings so i'm not sure if the name is right. There are two layers to bindings to me, the API layer and the ABI layer. This targets the ABI side and leaves you to write the API side. But even then it doesn't quite handle the ABI layer for you. This is fine though as there are lots of potential ABIs we need to support.

My one complaint other than the name, and maybe I'm missing something, is I'm not sure this should be completely generated. It feels like it would be nicer to spell it out in a class myself that's in my codebase then you somehow bind the module to that by maybe peppering in some decorators to tell the wasm instance how to bind to it.

@electrum
Copy link
Collaborator

@bhelx take a look at Demo from #519

@andreaTP
Copy link
Collaborator Author

not sure if the name is right

Not attached to it in any way, happy to receive proposals!

I'm not sure this should be completely generated

At the moment, I'm generating an abstract class that forces the user to implement the host import functions with the correct signatures, this is an example of what the user is expected to provide.

Let's discuss further how to proceed on this subject, but this PR can easily turn into an annotation-processor similar to #519

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.

3 participants