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

Initial implemention of bindgen for exported resources #232

Merged
merged 3 commits into from
May 9, 2024

Conversation

jamesls
Copy link
Contributor

@jamesls jamesls commented May 7, 2024

Issue #197

This adds support for generating bindings for resources exported from a
wasm component, so that Python code can interact with these resources.

I looked over jco/componentize-py and followed the same overall
approach. The internal structuring of bindgen.rs here is slightly
different than jco, so I erred on the side of keeping things as they are
as much as possible.

In terms of the generated code itself:

  • A resource maps to a Python class.
  • Calling the constructor of a resource returns a new instance of a
    Python class.
  • Protocols are generated for each resource in the module corresponding
    to the exported interface.
  • The protocols follow the existing calling convention of
    `func(caller: Store, ...) for exported functions.
  • The instantiation of the resource class is accessible from the instance
    of the class that holds the scope for the exported interface.

One of the issues I had to work out was how to pass in the root object
that contains all the trampolines needed by the resource classes, which
is defined in the root object. This is an issue because the
constructor of a resource is modeled in WIT, and the root object is
typically passed as a param in the __init__. What I ended up doing
was using a closure to capture the root component instance and then
dynamically generating the class when the interface "scope class" is
instantiated. For example, given this WIT file:

package component:basicresource;

interface my-interface-name {
    resource demo-resource-class {
        constructor(name: string);
        greet: func(greeting: string) -> string;
    }
}

world example {
    export my-interface-name;
}

The interface module for my-interface-name would have:

class MyInterfaceName:
    component: 'Root'
    
    def __init__(self, component: 'Root') -> None:
        self.component = component
        self.DemoResourceClass = _create_demo_resource_class(component)

# ...

def _create_demo_resource_class(component: 'Root') -> type[DemoResourceClass]:
    class _DemoResourceClass:
        # Preserve the signature defined in WIT.
        def __init__(self, caller: wasmtime.Store, name: str) -> None:
            # Capture the root component for access to all the trampolines/canons.
            self.component = component

And the code to instantiate a resource looks like this:

from wasmtime import Store

import mybindings # Whatever name you used.


store = Store()
root = mybindings.Root(store)
interface = root.my_interface_name()
instance = interface.DemoResourceClass(store, "myname")
print(instance.greet(store, "Hello"))

Would love feedback on this, it's unconventional in Python, but I suppose it
also maps closer to the idea of a(resource ...) being a type constructor
that creates a fresh type for each instance, while still being able to use the
the module level protocols for the resource to satisfy type checking of the
resources.

Imported resources aren't implemented as part of this change. Trying to
bindgen imported resources will still result in an unimplemented!()
panic. Wanted to get feedback on the overall approach before going
too far.

As part of this change I also added a commit to implement more of the WASI
shims needed to run bindgen (same stack trace from #202).

jamesls added 2 commits May 7, 2024 12:57
This replaces the `NotImplementedError` with enough of an
implementation to run the bindgen on components with resources.
This gets us further into the bindgen code, which now panics
at the unimplemented parts for resources.
This adds support for generating bindings for resources exported from a
wasm component, so that Python code can interact with these resources.

I looked over jco/componentize-py and followed the same overall
approach.  The internal structuring of `bindgen.rs` here is slightly
different than jco, so I erred on the side of keeping things as they are
as much as possible.

In terms of the generated code itself:

* A resource maps to a Python class.
* Calling the constructor of a resource returns a new instance of a
  Python class.
* Protocols are generated for each resource in the module corresponding
  to the exported interface.
* The protocols follow the existing calling convention of
  `func(caller: Store, ...<modeled args>) for exported functions.
* The instantiation of the resource class is accessible from the instance
  of the class that holds the scope for the exported interface.

One of the issues I had to work out was how to pass in the root object
that contains all the trampolines needed by the resource classes, which
is defined in the root object.  This is an issue because the
`constructor` of a resource is modeled in WIT, and the root object is
typically passed as a param in the `__init__`.  What I ended up doing
was using a closure to capture the root component instance and then
dynamically generating the class when the interface "scope class" is
instantiated.  Would love feedback on this, it's unconventional in
Python, but I suppose it also maps closer to the idea of a `(resource
...)` being a type constructor that creates a fresh type for each
instance, while still being able to use the the module level protocols
for the resource to satisfy type checking of the resources.

Imported resources aren't implemented as part of this change.  Trying to
bindgen imported resources will still result in an `unimplemented!()`
panic.  Wanted to get feedback on the overall approach before going
too far.
Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

The class-in-function idiom looks fine to me. It's not common but I wouldn't say it's unidiomatic either; just rarely used.

One remark on it is that the name of the function ends up a part of the qualified name of the class, so you might want to name that something sensible because it will end up in error messages and debug output everywhere.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks so much for helping to work on this! I've only got one minor comment below, but otherwise I think this all looks quite reasonable to me and would be happy to merge.

The main thing I might recommend perhaps bikeshedding a bit on is the testing story here. Tests so far in this repository are raw inline text-format-components which aren't exactly readable or easy to work with. Do you have thoughts/ideas on how to improve this? For example it might be reasonable to use componentize-py here to help generate test cases?

Ideally I'd imagine that test cases are a WIT world plus a "thing" which is a component, where the "thing" is probably ideally Python given this repository but could be anything that compiles to a component. That would make things much more readable and easier to write tests I think

rust/src/bindgen.rs Outdated Show resolved Hide resolved
@jamesls
Copy link
Contributor Author

jamesls commented May 8, 2024

The main thing I might recommend perhaps bikeshedding a bit on is the testing story here. Tests so far in this repository are raw inline text-format-components which aren't exactly readable or easy to work with. Do you have thoughts/ideas on how to improve this? For example it might be reasonable to use componentize-py here to help generate test cases?

Interesting idea, I'll try and sketch out writing tests using componentize-py and see how it goes. If it works out, it would be easier to write tests compared to writing wat.

@alexcrichton
Copy link
Member

I'm really liking how #234 is shaping up, thanks again for that! In the meantime no need to block this on that, so I'm going to go ahead and merge this. Thank you again for your work here!

@alexcrichton alexcrichton merged commit 7316692 into bytecodealliance:main May 9, 2024
11 checks passed
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