SIP-024: spin deps cli dx

[fibonacci1729]

This SIP proposes a Dx for guiding a user into adding a dependency to a component in a Spin.toml. It builds on #3445 and the implementation #3450.

[itowlson]

LGTM. I like the capabilities stuff; in fact I almost wonder if I should be able to run the capabilities stuff outside of doing an add.

[itowlson]

This is not surfaced interactively, right? (I would like it not to be - I just want to check we are on the same page.)

oh wait, this is the interface on the left hand side, not the export = option? That coincidence of terminology is going to be a trap

[fibonacci1729]

They're equivalent when selecting interfaces. I couldnt think of a better name here that encapsulated packages and interfaces. How do you feel about --use?

[itowlson]

Wait once again it seems I am utterly confused about a feature we have been shipping for a gazillion years - I thought export was about remapping interfaces... sigh

I was initially going to suggest --interface, but I now realise that if you want all a package's APIs, you have to tell it the package name if you don't want it to interact. --use seems a bit open-ended but could be an option. We could try --import perhaps? It's not great though. Well maybe it's okay, I originally thought "ugh, importing exports" but well, huh, that is what importers do, so maybe it makes sense?

Or send up the BIKESHED BAT-SIGNAL and let Lann come up with something.

[fibonacci1729]

I think import works. We could also keep export for the remapping scenario you mention which is the point of it. All I meant was that if the LHS is an interface, it implies that an export with that name exists with that name so its technically equivalent.

[fibonacci1729]

I.e. "a:b/c" = { export = none, ... } is equivalent to "a:b/c" = { export = "a:b/c", ... }

[itowlson]

Aha, I think I see what you mean now. Thanks!

[fibonacci1729]

We could allow add a mutually exclusive --package/-p, --interface/-i.

[fibonacci1729]

Actually never mind, I forgot about plain names.

[itowlson]

Presumably these will be check boxes rather than a single-select?

[fibonacci1729]

Indeed

[itowlson]

Or --inherit allowed_outbound_hosts --inherit ai_models, right? (that is, as well as not instead of)

[itowlson]

I worry with this that we may be asking a question the user doesn't know how to answer. I want the authentication interface, is that in client or util? If I guess wrong, is there a way back?

Possible alternative approaches:

1. Show them a list of everything they could import - all the interfaces plus an "everything" for each package.
2. When they come to interface selection, offer a "The interface I'm looking for isn't here. Take me back to package selection" option

Or maybe this isn't a concern and we have high confidence that users will know what they are looking for and just want to save on ye olde typing, I am not sure.

[fibonacci1729]

Good call out. Ill have a tinker and see if i could smooth this out. I think listing everything with all options for each package is the way to go.

[rmarx]

I was thinking something similar, but you might run into an issue where there's just a TON of exports in a given dependency, and navigating them manually would get annoying.

Counterpoint is of course that that won't happen super-often and you only need to do this once/infrequently, so I'd also be in favor of having just 1 flat list of everything with the "All from DEP" in the list as just another option.

[fibonacci1729]

That's a good point @rmarx. I don't anticipate dependencies that will have an overwhelmingly large # of exports so I agree with your counterpoint.

[itowlson]

Nit: I would avoid saying 'export' here (because I want to import something, where is the choice for that). Consider e.g. "Which interface do you want to import?" or some better wording that avoids the whole import/export debacle altogether (a la "which interface do you want to use" although that's hardly going to set the poetry world alight either)

[rmarx]

I had a similar feeling when seeing the CLI argument is called --export instead of --import... it depends on how you want to look at it of course.

However, since I'm adding a dependency to a specific component, I probably want that component to import/use stuff from the dependency (I don't necessarily want to think about the stuff the dependency exports).

Kind of like a github PR should really be a Merge Request instead? :D (imo)

Either way, if we keep --export (instead of --import or --interfaces) I agree it would be more sensible to have some other language here like suggested by @itowlson

EDIT: just now reading the whole discussion on this below :)

[karthik2804]

Should we also support adding by component ID? I am imagining a case where we have a component that we already build as part of the manifest that we would like to add as dependency to another.

[rmarx]

Strong agree on this one. I don't want to have to know/copy-paste the build output path of my Rust component to add it as a dependency of my TS component in the same spin project. I want that to be auto-figured out from the component ID

[rmarx]

EDIT: just read the bottom of the doc saying that multiple exports are not yet in-scope. Decided to leave the comment though, as I do need we can/should make it a bit clearer in this doc/eventual documentation that it's intended you can only add 1 interface at a time.

Just want to confirm if/how multiple exports work. I assume that

--export aws:client/s3@1.0.0,aws:client/sqs@1.0.0
and
--export aws:client/s3@1.0.0 --export aws:client/sqs@1.0.0

would both work? Or is it always just 1 export at a time?

If there are multiple possible at once, then some text above needs to be adjusted (e.g., "Which exportS should be used").

If however you can only do 1 interface at a time, that can probably also be made a bit clearer in the text/description that people would run multiple spin deps add commands for multiple exports, even if from the same package.

[rmarx]

So is that inherit_configuration = [] or simply no inherit_configuration?

[itowlson]

@rmarx They are equivalent. (Well, there is an edge case in which they are not equivalent. But we should catch that first and not ask the question if we are in that edge case! @fibonacci1729 I'm thinking of where the component already has a dependencies_inherit_configuration field.)

[fibonacci1729]

Could you say more about the edge case? I'm not sure i'm following.

[itowlson]

The edge case is where dependencies_inherit_configuration is set. In that case, no inherit_configuration is acceptable, and inherit_configuration = [] is an error (because it implies "inherit nothing" which contradicts dependencies_inherit_configuration). So if DIC is already set on the component, spin dep add shouldn't ask about capabilities.

[rmarx]

Is this printed only for capabilities that were not inherited?

Would be confusing if you explicitly chose --inherit all or --inherit ai_models and then still saw this kind of message at the end?

The example below does indicate this warning would be present if inheriting, so I think that's not ideal.

[rmarx]

Reading it a bit more, it seems it's saying you might need to actually fill in ai_models on the PARENT component, even if you added it as --inherit here.

Conceptually that makes sense, but it's confusing imo.

Is there a way to check if the parent component has the capabilities already filled out and only print this type of warning if it doesn't?

[itowlson]

@rmarx Hmm, good question. If the dep used the llm API, you would need the parent component to have ai_models so that the dep had something to inherit, and you would need the dep to inherit that setting.

We need to print a warning if the dep uses llm and parent has ai_models but the user declined to inherit it. We might ideally print a slightly different warning if the dep uses llm and inherits ai_models but the parent doesn't set ai_models. And maybe another one if the dep doesn't inherit and the parent doesn't set it...!

[rmarx]

Yeah, strongly in favor of more fine-grained warnings here :)

Another thing I was thinking about just now re-reading this: what happens if the user removes something related to this from the .toml down the line (e.g., removes ai_models from the parent while keeping it as a requirement/inherits on the dependency). Will spin build do a similar check as spin deps add to detect that and issue a warning (as it probably should)?

[itowlson]

That's a good question. Naively, the behaviour would be that you inherit the parent's ai_models permissions, which are "nothing." So if the dep really did use the llm interface it would get an "access denied" error for all models at runtime, which... is correct, but the reason might be non-obvious. So yeah probably helpful to emit the same warning at build time as we would have done at add time. @fibonacci1729 what do you reckon? Effort?

[fibonacci1729]

I think the best we could do at build time is check if a particular capability was inherited and issue a warning iff the configuration is the empty set.

[rmarx]

Hey all,

Thanks for working on this important part of the spin ecosystem.

I added a few comments, but most of them are nits and only few have to do with the underlying proposal/design, which in general I think is solid.

I'm quite new to spin (Akamai employee working with the Fermyon team) so I might have made some wrong assumptions/basic errors (e.g., not knowing how external dependency resolution error handling is done, not knowing if there's a way to remove added dependencies, ...). Feel free to correct me where wrong :)

One of the bigger missing pieces in this proposal as a higher-level user is the tie-in with WIT and auto-generation of programming language bindings when a dependency is added. I reckon that's documented elsewhere / seen as a separate project (which is fine), but I might have expected just a bit more tie-in to how that's supposed to interact here. Currently the text just talks about re-generating the spin-dependencies.wit file, but it's not entirely clear what that does exactly/how that flows into getting the dependency actually ready for use from within other components. If another command is needed (say... spin deps bind who knows) it would be good if that next step would e.g., be mentioned in the output text after a spin deps add potentially).

[itowlson]

@rmarx Thanks for asking about how interfaces flow from spin-dependencies.wit into application code. It's a good question and not well documented. The answer is that Spin punts this to languages.

For example, the Rust SDK has a macro that generates bindings from spin-dependencies.wit if it exists. I think Karthik developed something that sits in the JavaScript toolchain although I am not sure of details. So the intent is that it should magically work if you use the templates. We're not there yet - I don't think all templates are enlightened (possibly none are yet, I think JS stuff is a bit in limbo over WASI P3 stuff). But the basic principle is that Spin makes the dependencies WIT available to the language tools, and the language tools can pull that in in whatever way makes sense for that language. That avoids Spin itself having to know how to generate bindings for however many languages support the Component Model!

So all of that is out of scope for this SIP, but it's fair to say the reason it's out of scope is folk knowledge rather than explicitly documented.

[karthik2804]

Here is test case as sample for how it will work in JS - https://github.com/spinframework/spin-js-sdk/tree/main/test/deps-test

https://github.com/spinframework/spin-js-sdk/blob/643f08206354bc43e24ac52d60adc3b9822c559d/test/deps-test/package.json#L8-L10 is the important bit where we have a NPM script for generating the bindings that gets called with the build step first before building the component.

[rmarx]

Hey @itowlson and @karthik2804, thanks for the added context here :)

While I of course still agree this is/should be a separate thing, I'm not entirely agreed with "that is out of scope for this SIP"... (though all I'm really asking for is a "Don't forget to run spin build to generate language bindings" part of the output :))

In one of the comments above where I asked about maybe needing a spin deps remove, I got the impression that updating spin-dependencies.wit during spin deps add was mostly to provide IDE support for the new dependencies before a new spin build was executed.

With Karthik's example above though (and hopefully correctly interpreting @itowlson), it's clear that the generation of the language bindings only happens during the build process. Put differently IIUC: after spin deps add the programmer would conceptually get partial IDE support via spin-dependencies.wit but would need spin build for "full" IDE support in the programming language.

If this is correctly interpreted on my end, then I think we can make this clearer to the programmer by explicitly mentioning the recommendation to run spin build after spin deps add.

To re-iterate, I think it's fine to do the binding generation etc. at build time. I'm just saying that it might be confusing to newer users that are unfamiliar with the ecosystem (and that are maybe just copy-pasting a spin deps command from a README), that the dependency isn't immediately "fully" usable (e.g., from a TypeScript component because the bindings haven't been generated yet). I think I can say that fairly confidently since I remember how confused I was with all of this a year ago (and still a bit tbh :D).

TL;DR: if spin deps add doesn't do the complete logic that is needed to make the dependency directly callable from the parent component in userland code, imo that should be clear(er) to the end user.

[itowlson]

it's clear that the generation of the language bindings only happens during the build process

Well - and I apologise for the pedantry - it depends. But yes in the general case the user might need to build or refresh their IDE or whatever to get the bindings to regenerate. (The Rust language server is maddening in that in theory it can pick up changes without a build... but in practice it often doesn't pick up changes even after a build, because the build and language server are completely independent. Whereas in JS the IDE will never pick up without a build - barring watch shenanigans - but a build will reliably make them available in the IDE.) So we probably want a dreadfully vague and generic message along the lines of "you may need to build or reload the project to see your bindings."

I hope that makes sense - I do sympathise with the confusion!