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

New wac plug command #93

Merged
merged 4 commits into from
Apr 26, 2024
Merged

New wac plug command #93

merged 4 commits into from
Apr 26, 2024

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Apr 19, 2024

Closes #80

This PR introduces the wac plug command. wac plug takes any number of "plug" components and a single "socket" component. It then attempts to find all exports in each plug that have a matching import (both by name and type) in the socket. It then composes all the components together by filling in the sockets imports with the exports from the plugs.

All the exports of socket will be exported from the composition, and any unused exports of the plugs will be dropped. All imports from the plugs and any unplugged imports from the socket will be imported into the composition.

Example

// hello.wasm
package example:hello;

world hello {
    export hello: func() -> string;
}
// greeter.wasm
package example:greeter;

world greeter {
    import hello: func() -> string;
    export greet: func() -> string;
}

We want to compose these two components into a component with the following wit:

package example:greeting;

world greeter {
  export greet: func() -> string;
}

To do this, we can run:

wac plug --plug hello.wasm greeter.wasm -o out.wasm

The goal of the command is to treat one component as a plug which will
have all of its exports that match the imports of another component, the
socket, plugged into the socket component. The exports of the socket
will be re-exported.
Signed-off-by: Ryan Levick <[email protected]>
@rylev rylev requested a review from peterhuene April 19, 2024 15:56
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

Nice work!

It's great to see how relatively easy it was to implement this on top of the CompositionGraph API.

In addition to my two comments, I have some general feedback:

  • Regarding the "plug" and "socket" nomenclature (including the plug subcommand name), I wonder if perhaps we use a "link" verb which takes a "root" component and a set of "dependencies" to satisfy instantiations with; to me there's enough of a distinction between this and "composing" (i.e. using a WAC source file).

  • While this implementation is definitely a step forward (truly!), it differs in a significant way from the -d option to wasm-tools compose; chiefly, the -d option registers a component to use as an argument to any instantiation being processed. This is especially required for a wasi-virt use case where you might have a component that exports WASI instances and want that plugged into every other instantiation (including dependencies) to get the same WASI implementation used.

With the second point above in mind, I'm thinking this command might work like this:

  • Load each dependency package and walk its exports, registering each one in an "exports" map and treat it as an error if more than one dependency exports the same name.

  • Starting with the root component, satisfy any arguments with the registered exports while keeping track of a singular instantiation for each dependency (when used); for each of those connected arguments, recursively walk those instantiations and satisfy their arguments from the registered exports, with any unsatisfied argument remaining an outer import.

  • Gracefully handle a graph cycle error during encoding as it is certainly possible to form a cycle this way (dependency A imports i and exports e, dependency B imports e and exports i, root component C imports i, invoked with, perhaps, wac link -d a.wasm -d b.wasm c.wasm):

    flowchart TD
      A[A] -->|e| B[B]
      B -->|i| C[C]
      B -->|i| A
    
    Loading

What are your thoughts on this?

src/commands/plug.rs Show resolved Hide resolved
src/commands/plug.rs Outdated Show resolved Hide resolved
@peterhuene
Copy link
Member

peterhuene commented Apr 19, 2024

Also, it would also be great if this command could additionally support the registry feature so that the option for the dependency could be based off of a package name, like -d foo:[email protected].

That could potentially be a follow-up feature and doesn't necessarily need to be part of this PR, though!

@rylev
Copy link
Collaborator Author

rylev commented Apr 22, 2024

Thanks for the feedback!

  • Re: "plug" nomenclature vs. "link"
    • One of my goals with this subcommand is to unlock the most simple composition use case: sticking two components together. At the core of this is a deep desire to get away from some of the nomenclature used in wasm-tools compose which I found extremely confusing when I was first exposed to it - in particular the idea of "dependencies" which took me quite a long time to understand since this is an idea that is not directly reflected in components and is a somewhat overloaded term. To be clear, the nomenclature makes sense when the user's mental model matches correctly, but I worry that most users won't have this mental model when approaching wac as a casual user.
    • One of the promises of the component model, is the idea of composing components like lego bricks which I think first perfectly when taking the, admittedly, simplified view of composing components 1 to 1 . I believe the plug nomenclature is best when operating in that mental model.
    • That being said, I'm not tied to the "plug" nomenclature itself. The higher order bit for me is the 1 to 1 export into import semantics for which IMO a plug metaphor fits nicely.
  • Re: matching wasm-tools compose -d semantics.
    • Hopefully, I've made it clear above the mental mode I was targeting with the plug command and why I chose the simplified 1-to-1 semantics over the more powerful dependency oriented semantics of wasm-tools compose.
    • Even for the more complicated use case of dependency itself depending on another dependency, I think for many users (especially those with less exposure to the component model), multiple calls to wac plug would be preferable to a single call. For example, wac plug --plug wasi-virt.wasm my-dependency.wasm -o intermediate.wasm && wac plug --plug wasi-virt.wasm --plug intermediate.wasm my-component.wasm
    • I 100% see the practical use case for the more powerful, dependency-oriented approach though. I understand the hesitation around having two commands, as at the end of the day, they are fairly similar and having two commands could cause more confusion. At the end of the day, this feeds into question of how many commands we actually want that is touched on in Feature idea: short hand commands for common composition scenarios #53.

So I suppose the following are the questions now at hand:

  • Do we have agreement that the plug metaphor is more approachable for newcomers than the dependency oriented approach? If not, we should figure out how to align at that point as I think every decision afterwards stems from the perspective we take on this question.
  • How do we feel about the possibility of two different commands? Would this cause more confusion than it solves?
  • If we want to settle on one command, it probably makes most sense to settle on the more powerful dependency based approach, but how we can ensure that such an approach is understandable to all users.

@lann
Copy link

lann commented Apr 22, 2024

Food for thought on the name: wasm-tools component link is already a (completely different) thing...

@peterhuene
Copy link
Member

One of the promises of the component model, is the idea of composing components like lego bricks which I think first perfectly when taking the, admittedly, simplified view of composing components 1 to 1 . I believe the plug nomenclature is best when operating in that mental model.

Cool, I'm onboard with this; we can always change it in the near future if it proves confusing to users.

Hopefully, I've made it clear above the mental mode I was targeting with the plug command and why I chose the simplified 1-to-1 semantics over the more powerful dependency oriented semantics of wasm-tools compose.

Gotcha. I originally thought it was meant to be a 1:1 replacement of wasm-tools compose -d, but this simplified model I think works great. We can always add an additional command to do something a little more complex.

Do we have agreement that the plug metaphor is more approachable for newcomers than the dependency oriented approach? If not, we should figure out how to align at that point as I think every decision afterwards stems from the perspective we take on this question.

Yes, I think "plug" makes sense in this context.

How do we feel about the possibility of two different commands? Would this cause more confusion than it solves?

I'm fine with it; it feels like "plug" is simpler but more explicit while the "link" command I propose above is more complex but magical (and may not get everything right). I think there's room for two commands should need arise.

If we want to settle on one command, it probably makes most sense to settle on the more powerful dependency based approach, but how we can ensure that such an approach is understandable to all users.

That is indeed a tradeoff; many users will probably expect the "plug" behavior to start with. My thoughts on this is that we move forward with your implementation and see if there is additional need for automatic composition.

Signed-off-by: Ryan Levick <[email protected]>
@rylev
Copy link
Collaborator Author

rylev commented Apr 24, 2024

@peterhuene awesome! I've addressed the small feedback you left inline. I can add registry support in a follow on PR.

@rylev rylev requested a review from peterhuene April 25, 2024 08:52
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

Just a few more feedback comments, but otherwise looks great!

src/commands/plug.rs Outdated Show resolved Hide resolved
src/commands/plug.rs Show resolved Hide resolved
src/commands/plug.rs Outdated Show resolved Hide resolved
src/commands/plug.rs Outdated Show resolved Hide resolved
Signed-off-by: Ryan Levick <[email protected]>
@rylev rylev requested a review from peterhuene April 26, 2024 11:48
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for adding this!

@peterhuene peterhuene merged commit 3a4eba4 into bytecodealliance:main Apr 26, 2024
7 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.

Implement a simple composition flow a la wasm-tools compose -d
3 participants