Constructors

[harsh comet]

@heavy mulch, what's your preference on having more sdks in TestModuleConstructor? It's nice to have the module code colocated with the assertions but I'll either have to for...range on each sub-run, or one bigger for...range but putting all 3*2 module sources next to each other in the beginning, before looping.

[heavy mulch]

Whatever makes your life easier 🙂 At our current stage the only thing I honestly care about are that the tests exist and run, we don't need to optimize for a pretty-looking hierarchy of tests or anything

If in the future we do some refactorization of the integ test suite as a whole in order to bring more structure then we can reconsider

[harsh comet]

My concern is maintainability and what's easier to read and change.

I think I'll go for a single loop and flattened to go: basic etc.

[heavy mulch]

Yeah I agree but in the current setup I think any approach ends up close enough the same, to get better overall readability+maintainability I think we need larger scale changes, which is out-of-scope

[heavy mulch]

So yeah that sounds good to me 🙂

[harsh comet]

I'm getting something weird in the output and maybe I'm just tired but... I ported the "basic" example to Python and run with:

dagger call --foo=abc foo

I get "abc" as expected. But if I do

dagger call foo

I get <nil> 😐

Looking at the debug statements is what gets me confused...

dagger call --foo=abc foo
...
┃ [DEBUG] dagger.mod._module: invoke => {'parent_name': 'Test', 'parent_json': 'null', 'name': '', 'input_args': {
┃ 'bar': 123, 'foo': 'abc'}}
┃ [DEBUG] dagger.mod._module: output => '{"foo": "abc", "dir": [...]

I'd expect there to be two invokes:

1. constructor
2. foo field

I just see the debug for one, but it returns the field -> "abc" in final output.

Hmmm... why is bar even there in the input_args?

[heavy mulch]

That's especially odd, there's some issues around excessive caching that we need to fix (in linear), try doing docker rm -fv dagger-engine.dev; docker volume rm dagger-engine.dev and then running the tests again to see if that changes anything

If you haven't already

[harsh comet]

No, I haven't. Let's see...

[heavy mulch]

There only being 1 invoke is expected because we don't invoke modules to get the value of fields, only functions

[harsh comet]

Oh, I wasn't even aware of that. So Python's FieldResolver never gets called?

🤣

[heavy mulch]

Yeah we set the resolver for fields to be the "trivial" one where it's just read direct from the parent object server-side: https://github.com/sipsma/dagger/blob/6708004b3217c75c47e261836d79863cb95b9fdd/core/schema/module.go#L856-L856

Since it feels silly to do a whole module function call just to read a field we already have access to

[heavy mulch]

I'm guessing that might explain the nil? 🙂

[harsh comet]

Yes, of course. It makes total sense, somehow missed it 🙂

[harsh comet]

Yeah, I think the problem is that when the python objects are serialized, it skips fields that aren't set. So a default value won't be sent when I return the instance and since the server doesn't have that value, it returns nil.

It's usually preferable to keep the defaults out of the serialization, maybe it's ok in this case, just haven't thought much about it yet as I expected the field resolver to hit a new instance.

[heavy mulch]

Hm yeah, I mean ultimately that trivial resolver is just a performance optimization, so it's not strictly necessary to be in place. But removing would also add overhead to SDK development just since it's another case to handle (at least in Go and similar langs where the serialization works out better)

Or maybe server-side just needs to handle default values in the trivial resolver?

I guess we don't have support for defaults in FieldTypeDef yet, I suppose we should

[harsh comet]

I meant that it's usually left out not for performance reasons, it's usually to know what's been set or not. A recent example was a bug where using dagger.PortForward was sending its defaults when the server had to know the diff between nil and unset so I had to make sure the defaults weren't sent. This is a different case though, so I can just make sure everything's sent.

Yes, FieldTypeDef could have a default value.

Btw, after full wipe, I still get bar:

$ dagger call --focus=false --foo=abc foo
┃ [DEBUG] dagger.mod._module: invoke => {'parent_name': 'Test', 'parent_json': 'null', 'name': '', 'input_args': {
┃ 'bar': 123, 'foo': 'abc'}}

[heavy mulch]

I'm actually second guessing myself, I don't think graphql supports default values on fields (which actually makes sense), I was misremembering. It's only args that can have defaults I think. This is tangential though, can shelve 😅

[heavy mulch]

🤔

[harsh comet]

I have an ideia.

I'm actually setting defaults on those fields because they're arguments to the constructor function and arguments have defaults.

So with_arg("bar", ...., default_value=123)

Then the server must be using that default when invoking the constructor.

Which is actually something I don't expect.

[heavy mulch]

Wait, so you are calling that with_arg on the function provided to with_constructor? Or on a different function?

[harsh comet]

On with_constructor

[heavy mulch]

Then shouldn't it be expected that defaults are honored? At least in terms of the server-side logic? I basically tried to make constructor as much the same as any other function as possible

[harsh comet]

Yes, but if a function has a default value I'd expect that not to be sent by the server if the value is the same. I'd prefer if from the language's side there's a change to trigger the defaults. Otherwise they're just registered on with_arg but never actually used when invoking, not by python.

[heavy mulch]

Oh okay, that's intentionally not the behavior today; if you put a default value in the schema and a caller invokes your function without providing a value for that arg, the server sets the default on your behalf. Not actually something we do explicitly, it just happens in the graphql server lib we use.

I would have actually thought that ended up making life easier for SDKs, what's the reason it's not preferred for python?

[heavy mulch]

Oh I think I understand what you're saying here maybe, this has to do with "self-invoking"?

[harsh comet]

It's not python per se, it's me. Since python supports defaults in both function arguments and class fields, I'm used to handling them in a certain way. It's very common to need to know when something's unset, even if the value is the same as the default since that default isn't immediately available in the function body without resorting to introspection. It's usually done with a sentinel value (something that you know no one will use). There's also the ability to trigger a default by a callable/factory or something more dynamic that you might want to regenerate locally without depending on an external serialized value.

So it may break some expectations depending on usage, but it may also be a edge case or not an issue at all in practice.

[heavy mulch]

Okay I think I get it, kind of related to the common more general problem of distinguishing explicitly set zero values and implicitly set zero values (but "default" rather than zero and intertwined with other python things in this case). We could in theory add a bool field here indicating whether the value was explicitly set or was just the default: https://github.com/sipsma/dagger/blob/8093994c826054eb25272c234a76ae65d1aa3cad/core/schema/internalsdk.graphqls#L48-L48 ?

I mean not immediately, just if this turns out to be impactful then that's one possible route to addressing

[harsh comet]

Yep, let's deal with it when we have to cross that bridge 👍

It's clearer to me now, I had a few incorrect assumptions.

[harsh comet]

I realized that the defaults thing is only being hit with Python now. I'm talking about the defaults of function arguments. Go doesn't support defaults (only if it's optional) so they're not being registered in WithArg, thus when the constructor is executed they're unset and the constructor does its thing.

I prefer to not send the defaults as part of a function call, in inputs. Ok for the field resolver in the server to use it, but if the runtime is being executed anyway, I'd rather leave the defaults to the implementation.

[heavy mulch]

I 100% think it should be okay for an SDK to choose to handle setting defaults itself, but I am not sure whether that means the server should never set them vs. just telling the SDK whether an arg value was explicitly set by the user or not (i.e. the bool field suggested above). I would probably lean towards the latter since it feels like generally a nice thing for the server to take care of this on the SDK's behalf, but we don't have enough SDKs to have very much data to support/deny that assertion right now 🙂

But to confirm, if the server told SDKs whether the arg value was explicitly set or not, that would be enough for Python to deal with this? Basically, if it wasn't explicitly set, the python sdk can just take care of defaults itself, ignoring any pre-populated value?

[harsh comet]

It's possible to get the defaults and compare the value with what's coming from the function call's inputs and not instantiate when those values are the same, but it's unnecessary overhead with not enough of a benefit to justify it. Similarly, checking if something is set or not to decide if it's included in the function's arguments is much trivial to do, but still feels unnecessary on the SDK's side.

If it's simple to change the server to not send the defaults, I prefer adding them back if needed in a future SDK rather than going around it for the ones in front of us now. Since the defaults are provided by the SDK itself, I personally can't find a reason to just not send those defaults in the first place. When would an SDK be able to set defaults for function arguments and not be able rely on them on execution?

If you use a separate system from the function's signature (or whatever it's used for registration) to set the defaults and then rely on the API to get them in the function call, you're missing out on using the defaults from other functions within the module. So you'd likely want to use that "external" system inside the constructor for that. That's what Go does and that's why I don't see a point even for languages that don't support default values.

It just feels cleaner to not send them in the first place. Less bytes over the "wire", less unexpected, and lets the SDK handle some dynamic situations more naturally. 🙂

On another point, I'm not sure if Python is going to support making API calls in the constructor. I can support it in some cases, especially when the SDK is in control of the instantiation such as invoking the constructor for the module, but when instantiating from within another function, it would feel alien so I'm not inclined to support it. The only good alternative I can think of is to declare a classmethod as an alternative constructor to override the default one but that would allow it to be async.

[heavy mulch]

When would an SDK be able to set defaults for function arguments and not be able rely on them on execution?
SDKs definitely always could, it's more a matter of whether in general that's putting more work on them or not. Go is probably going to support defaults in the near-ish future (https://discord.com/channels/707636530424053791/1177373379255349248), at which point I think it would need a little extra code to handle both providing the default value as part of the schema (which we still probably want to do for documentation purposes) and then also setting it during invocation time. But honestly it's not that much extra, so it's mostly splitting hairs for Go I think.

If it's simple to change the server to not send the defaults
I think it's not that hard; basically the server checks if the value of the arg it's about to provide is the equal to the default value and then unsets it if so. That's why it kind of feels sort of strange from the server's perspective; it's like "I could just give you the default value, but instead I'm going to hide it from you". That doesn't invalidate any of your points, it's just why I feel conflicted about it.

cc @timber lance @upbeat skiff in case you have any opinions on all this

[timber lance]

personally i think it might make most sense to allow reading the default arg? that way SDKs that want to (and actually have default args, RIP Go) can pull that data into their codegen

i think that's what would result in the nicest experience when working in IDEs that have nice autocompletion, then it becomes a lot clearer to the user what defaults are being selected (i don't think it matters hugely if the default isn't actually being set by the dagger server, it feels a bit weird, but i think it's made ok by the fact that the sdk experience is made better for those cases).

[harsh comet]

My disagreement is specifically about calling a function. In other words, not including default values in the function arg inputs in a call context.

[harsh comet]

SDKs definitely always could, it's more a matter of whether in general that's putting more work on them or not. Go is probably going to support defaults in the near-ish future (⁠ETOOMANYARGS), at which point I think it would need a little extra code to handle both providing the default value as part of the schema (which we still probably want to do for documentation purposes) and then also setting it during invocation time. But honestly it's not that much extra, so it's mostly splitting hairs for Go I think.

My point is that if an SDK supports default values in function arguments, you'd also want to have them when calling from inside the module right? If that's the case, it's not extra work if the server doesn't send the defaults because even without the API you want them to work internally. Doesn't seem like having the API send them saves work. And the extra work is only for languages that don't support defaults natively. It's the case in Go.