#ETOOMANYARGS

1 messages · Page 1 of 1 (latest)

jaunty oyster
#

I won’t be properly online until tomorrow but just read through quick, the idea in my head has been that once we can handle custom structs as inputs that would be the route to addressing these sorts of issues. So rather than something hyper specific like the Opts struct approach, more generally applicable and reusable.

The end goal would be that you can group related sets of args together into objects and accept those objects as inputs instead. You could make the structs as a whole optional, just individual fields in them optional, etc. So fairly flexible and just built on too of primitives also applicable to all sdks.

Now that we have support for custom objects as inputs on main, the missing pieces left are:

  1. Support for invoking functions with those from the cli
  2. Someway of dealing with the fact that programmatically those input structs themselves need to be constructed; I think there’s nice routes to this through some combination of automatic setter generation (brought up fairly often now) and/or extending the just merged constructor support to all objects (not just the main module object)
#

</braindump>

celest rampart
#

Sure, I'd have to see code but as long as it results in the same external API + CLI + Daggerverse docs and passable intra-module API I think it's on the right track. I don't think turning all the args into an input object (either GraphQL input or an ID-able object) is really a solution here since that'd result in a completely different GraphQL API than the original goal. Not sure if that's what you mean.

FWIW the Opts struct approach doesn't imply specifically looking for the word "Opts", it's more like flattening non-pointer structs in arg lists and treating their fields as args instead, which is roughly how it worked before. Not sure if that's what you meant by hyper-specific. The end result feels pretty 'native Go' to me, considering how many things use a similar pattern (including our own SDK, which admittedly has the variadic stuff on top, which I would avoid since that's painful on the authoring side)

placid isle
jaunty oyster
# celest rampart Sure, I'd have to see code but as long as it results in the same external API + ...

I don't think turning all the args into an input object (either GraphQL input or an ID-able object) is really a solution here since that'd result in a completely different GraphQL API than the original goal. Not sure if that's what you mean.
I think this is the crux of where I disagree. Whether the args are required or optional, I don't think it's ever a great interface to have a flat list of tons of them. It should be possible (we shouldn't completely prevent it), and it is possible today, but it's inherently not a great interface that I think we should go out of our way to encourage.

So if we add support in Go for flattening out optional args from a struct, I think all we've done is make it easier to create non-ideal APIs in one SDK. That feels like it should be more of a last resort if nothing else pans out rather than what we invest immediate effort into.

I think that grouping related sets of args (whether required or optional) into objects would:

  1. Result in users creating better APIs (not just long lists of flat args)
  2. Also handle the case of lots of required args (rather than be specific to optional ones)
  3. Not be specific to just Go
  4. Have more potential for integrating nicely with other potential features like automatic setters, more generalized constructors, etc.

Which is why I'd prefer investing effort down that avenue to start. Like I said, if it doesn't pan out then yeah we should do what it takes to make the DX better

#

I also btw don't think any of this rules out stuff like the default in the struct tag; it could actually fit in with that pretty nicely. I think we probably need our own dagger: struct tag for at minimum the ability to mark a field as private (another common feature request) and while the default would be inherently limited to whatever you can type as a string (like we talked about back then), it probably doesn't hurt at that point to support it imo

celest rampart
#

There's a chance we're talking about approximately the same end goal. The struct approach also supports required args for example. It sounds like what you're suggesting is to just not flatten them at the GraphQL API layer, but otherwise still use all the other struct-y mechanics (tags, named parameters, etc), which I would support in principle, but it feels like it's mostly solving a different problem (grouping related args to keep the toplevel set smaller). For example, what do I do when I just want to add a single optional arg to a function?

Also when it comes to the CLI I would argue that we should just flatten them into a big set of flags. It's totally normal for a CLI to have a ton of flags, and I definitely wouldn't want to have to somehow pass an object around in the CLI.

My biggest worry though is that we only solve for the purest, most ideal mode of writing modules. It's very typical to start writing a function and gradually add params as you go along. New users will most certainly run into this. If at any point along that journey you have to stop and refactor the world to introduce e.g. new stateful objects with WithFoo methods, or if whatever scheme you've got cooking up involves too much thinking, some users will just give up and condemn Dagger as too complex/opinionated. Sometimes it's nice to just be unblocked and build up a bit of tech debt, just adding one param after another. You always have the freedom to refactor it later. Especially considering some of these modules have only 1 user.

Most of what I'm talking about affects main modules more than library modules, btw. At least in spirit, I know there's no distinction and there might never be. I would agree with being a tad more purist when it comes to thinking about library modules.

jaunty oyster
#

There's a chance we're talking about approximately the same end goal. The struct approach also supports required args for example. It sounds like what you're suggesting is to just not flatten them at the GraphQL API layer, but otherwise still use all the other struct-y mechanics (tags, named parameters, etc),

Yeah exactly, it’s the automatic flattening support as a feature of a single SDK that I’d like to avoid. Otherwise I think the end result is not that different than what I imagine you’re imagining 😄

Also when it comes to the CLI I would argue that we should just flatten them into a big set of flags. It's totally normal for a CLI to have a ton of flags, and I definitely wouldn't want to have to somehow pass an object around in the CLI.

I also agree that for the CLI specifically flattening them out one way or another is one of the primary implementation options (probably others, but it’s the most obvious). The thing is, we do support objects as inputs now, so we’ll have to solve this no matter what and in such a way that it works for all module SDKs that the CLI could be invoking. So I just would like the solution there to be more generalized handling of arg types in the CLI rather than a feature we support in the Go SDK that happens to result in the SDK exposing flags in a way that the current CLI logic can handle.

For example, what do I do when I just want to add a single optional arg to a function?
My biggest worry though is that we only solve for the purest, most ideal mode of writing modules. It's very typical to start writing a function and gradually add params as you go along. New users will most certainly run into this.

Right that’s the thing, none of this stops users from just appending an optional/required arg to their list of accepted args. You don’t have to wrap everything up into a struct. So if you have 1 arg, need to add a new optional arg, you can of course just add it and now have a flat list of 2 args.

It’s the case you’re talking about where you’ve repeated the above process a bunch of times and it’s grown out of control that this matters. I just am advocating that we encourage users at that point to group related args together into objects rather than make it easy for Go SDK users to keep adding more args to the flat list.

If at any point along that journey you have to stop and refactor the world to introduce e.g. new stateful objects with WithFoo methods, or if whatever scheme you've got cooking up involves too much thinking, some users will just give up and condemn Dagger as too complex/opinionated.

Completely agree, if refactoring to group related args together into an object meant that you have to write all that annoying boilerplate, then it would be a non-starter. That’s where my (admittedly somewhat vague) ideas around automatic setters and generalized constructors comes in. I think there’s paths to making the whole process of adding a new custom object much much lower boilerplate, to the point that all you have to do as a module author is write the struct and accept it as an input (unless they really do want something custom, in which case they only need to write the code they actually care about, not the boilerplate).

And the thing is, we almost certainly want to do all that anyways, so my hope is that by solving these generalized problems we can also along the way fix the immediate DX problems you’re hitting (among pretty much all other users too).

#

Most of what I'm talking about affects main modules more than library modules, btw. At least in spirit, I know there's no distinction and there might never be. I would agree with being a tad more purist when it comes to thinking about library modules.

Yeah, another problem here is that all of the DX issues are intertwined, so it can be overwhelming to think about 😅 I would say that quite a few of the various open discussions end up touching on a difference between main+library modules, so it's highly possible that could end being involved here too. I will put a pin in that for the sake of not further expanding my wall of text yet

celest rampart
#

(Sorry to have kicked off a wall-of-text while you're supposed to have the day off, this can wait, I enjoy the discussion and hope it doesn't feel like a flamewar, I think we're already on the same page there as always but still want to say that anyway 😅)

Right that’s the thing, none of this stops users from just appending an optional/required arg to their list of accepted args. You don’t have to wrap everything up into a struct. So if you have 1 arg, need to add a new optional arg, you can of course just add it and now have a flat list of 2 args.

But this doesn't give you a way to set a static default (which is super common and very helpful to see in --help), and it forces you to use positional args rather than named args, which gets confusing as soon as you have more than one optional arg, and it's not always the case that some of your args can be grouped into an object to reduce the arg count.

My point here is only that solving for 'grouping related args together' doesn't address the root problem of how required/optional args are provided in Go.

Yeah exactly, it’s the automatic flattening support as a feature of a single SDK that I’d like to avoid.

The problem I'm talking about is specific to the Go SDK though. Python doesn't have this issue because it already supports named params + static defaults. There are always going to be 'features' that are only implemented by a single SDK because not every language is working with the same constraints.

To be clear, I'm not against adding support for structured input args, and I'm OK with it being prioritized sooner, and I can see how it benefits other SDKs too, but I don't think it will solve the issue with the Go SDK specifically. It's an orthogonal problem in the same space, and it could still be supported with an args type in the Go SDK by just having a field in the args type whose value is a struct (or whatever its value would have been as a positional arg).

Now that I looked this up, it seems like a few "GraphQL Best Practices" references go even further and advise against even having fields that take > 1 argument, suggesting that you jump straight to a single input type arg at that point. That has somewhat significant implications for our use case, so I wouldn't treat it as gospel, but it feels like a middle ground between what you're proposing (use inputs to group together related args) and what I'm proposing (which is approximately to use an input to wrap all arguments). The difference is that GraphQL doesn't do the "flattening" at the top-level, whereas we might want to just so languages that support positional args can use them natively.

#

(Caveat: "GraphQL best practices" aren't exactly authoritative/exhaustive, best I can do is some dude's repo with 4 stars, and ChatGPT, and vague references in blog posts with possibly unique motivations, but the comparison still seemed worth bringing up.)

jaunty oyster
#

(Sorry to have kicked off a wall-of-text while you're supposed to have the day off, this can wait, I enjoy the discussion and hope it doesn't feel like a flamewar, I think we're already on the same page there as always but still want to say that anyway 😅)
No it's all good! Alice is working today and I already played video games to the point of boredom this morning so I'm online now because I want to be/have nothing better to do (whatever that says about me personally, so be it 😆 )

And yeah these discussions are A) about incredibly non-obvious things that need back and forth to figure out and B) always end up being fruitful so I'm glad we're having it 🙂

But this doesn't give you a way to set a static default (which is super common and very helpful to see in --help), and it forces you to use positional args rather than named args, which gets confusing as soon as you have more than one optional arg, and it's not always the case that some of your args can be grouped into an object to reduce the arg count.
Right, so if I'm understanding you're getting at the fact that you have struct tags to supply metadata like defaults for struct fields, but not for flattened args? If so, agree I was glossing over that and it's important.

Honestly, just thinking out loud, what @placid isle posted above is extremely intriguing in terms of a solution here. Basically, struct tags in Go are fundamentally just glorified comments: a string that gets interpreted by some library doing reflection/ast parsing.

So at that point it feels like it would make just as much sense for us to expose all this via comments with a special prefix as it would a struct tag with a special dagger: prefix. But the advantages of comments are:

  1. They can be applied to not just struct fields but also args (like we already did for doc strings), functions, structs as a whole, etc.
  2. They can be split across lines, which would address one looming concern I didn't mention yet which is that stuffing everything into a single-line struct tag could get overwhelming if you need to supply a lot of metadata.

So if we took that approach, then you'd be able to get all the same features on a "flat arg" as you would an arg wrapped up in a struct, which I think would make that whole aspect of the problem orthogonal with the grouping of args into structs.

celest rampart
#

I could buy that! The only downside really is that intra-module calls have to contend with positional args instead of named, but that's really not a Dagger specific problem, and would probably be mitigated in most cases by grouping inputs, so it nudges the user in the right direction. (Also maybe dag.Self() will show up someday 🤯)

The other issue we didn't really talk about but which actually caused a lot of pain was having to do foo_ := foo.GetOrDefault("xyz") and deal with the awful code completion. (Keep in mind you don't want to rename the foo arg to fooOpt because that'll be reflected in the API.) But as long as // dagger: lets you skip the Optional[x] wrapper, that problem is solved.

#

In fact I'd probably just avoid Optional[x] entirely and do something like // dagger:optional instead. You wouldn't want to explicitly set a zero-value in a comment, since that would bubble up to the API.

#

Maybe since these are already Dagger-specific APIs we can skip the dagger: prefix and do whatever is cutest, like // +default 123. Not sure how commonplace the K8s comment syntax above is.

jaunty oyster
# celest rampart In fact I'd probably just avoid `Optional[x]` entirely and do something like `//...

Yeah the interactions between the "type metadata" and the Optional type (whether they co-exist or not, and if so how they interact) is the last major question mark in my head... That's a much better and smaller scoped remaining problem to have at least.

The thing Optional lets you do is set a default values in code that couldn't be encoded in a default: -like syntax; so defaults for objects like Container/Directory/etc. or defaults that have some sort of complex logic (maybe based on other args, etc.). All I care about is that those use cases are still relatively straightforward in one way or another, so just need to sort out details there.

Maybe since these are already Dagger-specific APIs we can skip the dagger: prefix and do whatever is cutest, like // +default 123. Not sure how commonplace the K8s comment syntax above is.
Just for another piece of "prior art", in Go if you want to prevent a func from being inlined you have to add a comment //go:noinline above the func: https://dave.cheney.net/2018/01/08/gos-hidden-pragmas

So even the go compiler has resorted to this sort of pattern due to lack of decorators laughcry I could go either way right now on whether we should do that style and have a dagger: prefix or more like the k8s style... Just throwing it out there. At least that's another much smaller scoped question than where we started!

Dave Cheney
Dave Cheney
Go’s hidden #pragmas
celest rampart
#

Yea, now we're just splitting hairs. 👍

My guess is Go went for scoped comments because it has to distinguish its comments from the full universe of possible comments in code. Seems like we have the luxury to take shortcuts in our case

re: Optional, my preference would be to just mark things with complex defaults as // +optional and reassign if it's empty like if foo == nil { foo = dag.Container().From("foo") }. That way I still just have the one var to worry about.

jaunty oyster
#

Cc @safe sleet (will make issue tomorrow)

jaunty oyster
jaunty oyster
# celest rampart Yea, now we're just splitting hairs. 👍 My guess is Go went for scoped comment...

re: Optional, my preference would be to just mark things with complex defaults as // +optional and reassign if it's empty like if foo == nil { foo = dag.Container().From("foo") }. That way I still just have the one var to worry about.
Yeah that makes perfect sense; would make it easier to do self calls until the day where dag.Self exists (which I agree probably needs to happen someday but is a pretty huge rabbit hole). I think we can leave in the Optional type for backwards compat if nothing else, should be able to co-exist peacefully. Whether we ultimately should in the long-term is TBD in my mind 🙂 It's always nice when there's only one way to do things of course

safe sleet
#

Caught up on this 🙂

So I just would like the solution there to be more generalized handling of arg types in the CLI rather than a feature we support in the Go SDK that happens to result in the SDK exposing flags in a way that the current CLI logic can handle.
I think this makes a lot of sense as an end-goal 🙂 Having this just be a generic case of object inputs seems nice - it also imposes hierarchy, which means that we could group together "types" of inputs, like "here are the auth parameters", "here are the database parameters", etc.

Out of curiosity, any specific ideas for what this might look like in the cli? getters/setters feels like it helps us here, but general construction of objects in the cli feels like it's worth diving into before we decide to rely on it 😄

Basically, struct tags in Go are fundamentally just glorified comments: a string that gets interpreted by some library doing reflection/ast parsing.
Hehe, I'm glad this was suggested - I had the same idea independently on vacation and wrote it down to bring up later. This should be super trivial to do actually once we bikeshed a syntax, I can try and add something tomorrow if no one beats me to it.

If we do this, should we keep the inline struct approach (to allow using tags), or should we remove it, and just allow using fields+comment parsing? To quote from above: "It's always nice when there's only one way to do things of course"

I think we can leave in the Optional type for backwards compat if nothing else, should be able to co-exist peacefully. Whether we ultimately should in the long-term is TBD in my mind 🙂
SGTM - I'm not personally super attached to Optional, it also avoids the recent thing about OptEmpty feeling a bit unweidy (I agree on that, but can't find the thread where that got discussed). We did get some good feedback from Optional though, so I definitely think we should deprecate instead of remove to see if we can get a feel from early adopters.

jaunty oyster
#

Out of curiosity, any specific ideas for what this might look like in the cli? getters/setters feels like it helps us here, but general construction of objects in the cli feels like it's worth diving into before we decide to rely on it 😄
Lots of options (with varying degrees of mutual exclusivity):

  1. "Flatten" out object fields but w/ prefixing; so if you accept an input myArg with type struct { Foo string }, then the CLI arg would be --my-arg-foo
    • Pretty sure this would be horrendously ugly to use in practice in the many/most cases, but technically "solves" the problem
  2. Allow annotations (as discussed above) that specify exactly how a given field in a function arg should be converted into a CLI flag (if at all)
  3. Don't support it; formalize the difference between main vs library modules and enforce that main module functions can't use objects as inputs (only libraries can)
  4. Don't support it; instead add support to the CLI for "SDK snippets" like dagger call --sdk python 'dag.my_func(my_arg={foo="fooval"})'
    • I'm forgetting python syntax, but the basic idea is you can write snippets of SDK code as a replacement for trying to coerce everything in posix-y flags + subcommands
  5. More I'm forgetting/not thinking of

I can try and add something tomorrow if no one beats me to it.
That sounds great! No one else is actively working on something with a ton of overlap I don't think

If we do this, should we keep the inline struct approach (to allow using tags), or should we remove it, and just allow using fields+comment parsing?
I'd vote to remove support for tags since I don't think it's too heavily used and all else being equal the less complication in that go sdk module codegen code the better 🙂 But this is a loosely held opinion

safe sleet
#

Don't support it; instead add support to the CLI for "SDK snippets" like dagger call --sdk python 'dag.my_func(my_arg={foo="fooval"})'
IMO we should do this regardless, it would be a super neat feature 🙂 would be super cool if languages that supported REPLs (like python) could drop you into one similar to dagger shell as well

celest rampart
# safe sleet > Don't support it; instead add support to the CLI for "SDK snippets" like `dagg...

@tawny blade proposed that ages ago 🙂 https://github.com/dagger/dagger/issues/5774 - and I agree going full REPL would be chefkiss and very possible now with the same mechanics as dagger shell

GitHub
[Zenith] `dagger script`: invoke any function from any module, in y...

Overview In #5757, we take the Zenith in a bold new direction: everything is a function functions are packaged together in modules each module has an API A module's API can be queried either 1)...