Go optional params

[vestal stream]

Currently have this working:

func Build(
    ctx dagger.Context,
    base *dagger.Container,
    src *dagger.Directory,

    // Packages to build.
    packages *[]string,

    // Optional subdirectory in which to place the built
    // artifacts.
    subdir *string,

    // -X definitions to pass to go build -ldflags.
    xdefs *[]string,

    // Whether to enable CGO.
    static *bool,

    // Whether to build with race detection.
    race *bool,

    // Cross-compile via GOOS and GOARCH.
    goos, goarch *string,

    // Arbitrary flags to pass along to go build.
    buildFlags *[]string,
) *dagger.Directory {
// ...

I'm thinking of switching this from pointers to accepting an arbitrary non-pointer struct arg, and treating the struct fields as optional params. Same exact way the SDK works

So instead you'd have this:

type GoBuildOpts struct {
    // Packages to build.
    Packages []string

    // Optional subdirectory in which to place the built
    // artifacts.
    Subdir string

    // -X definitions to pass to go build -ldflags.
    Xdefs []string

    // Whether to enable CGO.
    Static bool

    // Whether to build with race detection.
    Race bool

    // Cross-compile via GOOS and GOARCH.
    GOOS, GOARCH string

    // Arbitrary flags to pass along to go build.
    BuildFlags []string
}

func Build(
    ctx dagger.Context,
    base *dagger.Container,
    src *dagger.Directory,
    opts GoBuildOpts,
) *dagger.Directory {

(possibly supporting opts ...GoBuildOpts too because why not)

[indigo veldt]

That makes hella sense to me, it's great in that it'd be consistent w/ the existing approach in the SDKs. Let me double check what exactly will happen right now when you have a struct as an arg to a function, one min

[vestal stream]

there's some special-casing for GraphQL marshalable types like *dagger.Container, which is a bit borked because it actually checks if dagger.Container (non-pointer) implements the interface, which doesn't - easy fix. I think aside from that it probably blows up somehow haha, maybe just returns the struct type name or something

I can start working on opts struct support then 👍 most likely tomorrow. will push what I have to a branch

https://github.com/vito/dagger/commit/3358d60c0cdb61459bf0b5d91f2d7bcf9491099e

[indigo veldt]

Converting go type -> graphql schema type for structs will probably hit this branch where it converts it into a graphql input type: https://github.com/sipsma/dagger/blob/7e809c674b158cb72c2b584b370a442ba97e90f6/sdk/go/server.go#L295-L295

Which isn't unreasonable but not what we want here. Feel free to change that to split the struct fields up into optional gql args; that code path isn't used at all (only exists due to cloak vestigial remnants)

Other than that I think the rest of the code handles it in theory (in practice probably bugs but most likely nothing devastating)

[indigo veldt]

Sounds great, thanks!

[vestal stream]

@loud glade not sure if you've thought about optional params yet, but maybe the pattern here across all SDKs is to match the codegen UX, just like in this thread?

Unless there's something clearly more intuitive. Could vary from language to language.

[loud glade]

Oh, yeah, since I started the previous version... very early on 🙂 For optionals, in Python I expect the following:

def build(packages: list[str] | None = None):
   ...

So | None makes it optional, and = None gives it a default value.

[vestal stream]

Cool, that seems pretty natural. Is that how the python sdk codegen works too?

(maybe without defaults client side?)

[loud glade]

Yes it is 🙂

Yeah, we don't have any defaults in the API but that would be so.

This would still be optional, just because of the default value:

def deploy(where: str = "")
    ...

Since list is mutable, I can make a factory (to allow default empty list):

def build(packages: Annotated[list[str], dagger.Option(default_factory=list)]):
   ...

That's the same mechanism for flag descriptions and alternative names:

def hello(name: Annotated[str, "Who do you want to greet?"] = "World"):
   ...

Is syntatic sugar for:

def hello(name: Annotated[str, dagger.Argument(description="Who do you want to greet?")] = "World"):
   ...

This was already working in the previous version, just without the defaults. So 🙌 for that.

[vestal stream]

Found time to just implement this tonight. Pushed to sipsma/zenith, a bit hastily now that I think about it, feel free to push -f if everything breaks. 😅

What's great is this required very little code change vs the early mockup, just small tweaks. All of universe/go is converted now.

https://github.com/sipsma/dagger/commit/188ec53a109e0229b71f99f38f74cb86fcea9339#diff-01bff22a000b2c3d8a684bec8c0e255e153eb85adb10e903c32df439ebb810c0

[indigo veldt]

Just looked into detail, awesome! It might be kind of a stretch, but I'm wondering how much work it would be to add a Function to that env that takes a directory of *_test.go files, does the parsing to find all the Test* funcs (maybe there's something in the stdlib that does that already?) and then turns each of them into a Subcheck.

It'd be roughly along the lines of GoTest(base *dagger.Container, src *dagger.Directory, subpath string) (*dagger.EnvironmentCheck, error), internally calling .WithSubcheck for each Test.

I'm gonna work on artifacts first and then more demo polish, but if time after I may give that a shot

[vestal stream]

interesting, sounds fun, looking into it!

go test ./... -list=. -json appears to be the secret sauce, shouldn't be too difficult (. being a "match anything" regex)

@indigo veldt are you imagining it loops over all the tests and runs them individually, and puts all the results together? because another way of doing that could be to just run all the tests with -json and parse the output (same way gotestsum works), which would probably be more efficient.

At first I thought the idea was to construct a big set of not-yet-evaluated checks so you can list them all and choose which one to run, but I only see a EnvironmentCheckResult constructor, which is after-the-fact

[indigo veldt]

Yeah I was imagining that each test becomes a subcheck. The potentially nice aspects of that approach is that you'd get automatic parallelization (each subcheck would be its own exec) and potentially very fine grained caching. But you're right that there's also performance overhead to doing that (and problems if tests rely on global state between each other) relative to running them all at once and just creating a single result that aggregates them...

[vestal stream]

yeah, it sounds fun but I'd guess it'd perform worse than just regular old t.Parallel unless you were farming it out to a bunch of machines

maybe you could do just each package in parallel?

[indigo veldt]

Right exactly, which we will support someday, but obviously not yet

[indigo veldt]

Yeah that seems reasonable for now

Let me know if the API needs updates to make this work, I'm just realizing that it may be difficult to register subchecks with a func in your current codebase. The example I got working was registering subchecks that were sourced from the codegen bindings to other envs, which is good but I think I may need to append some more functionality for it to work here

[vestal stream]

yeah, it seems a bit tricky to run things in parallel and then register their results in parallel. easy with a lock of course, but that's a bit of boilerplate

my intuition here was actually to just append a bunch of Subchecks that are "primed" to run a Container, tbh

but I realize that's not how things are structured atm

it might be Bass influencing me here, which worked that way: https://github.com/vito/bass/blob/db53358b977cb5d7eace12b0a611ef13aa9e1ef7/bass/github-hook#L14-L19

[indigo veldt]

Right that's how it should work, but atm subchecks have to be EnvironmentCheck objects already, whereas you want to probably provide funcs that resolve each subcheck, same as how top level checks work

Let me try to add that really quick

[vestal stream]

ah yeah, funcs would be more general, that'd be awesome

[indigo veldt]

There's a restriction right now that they have to be funcs defined on the top level of the source file (the go reflection/ast parsing breaks down otherwise atm), but that should still work I think

[vestal stream]

assuming you also mean anonymous func() {}s? no idea how that'll work. 🤯

[indigo veldt]

that won't work quite yet, but we could support that, the hardest thing is that we need to do ast parsing to figure out the name of the arguments, but I think that'd be possible (just annoying)

[vestal stream]

would these ones take arguments? i assumed they were just closures, or are you thinking of parameterized checks?

(i can just wait to see what you cook up, maybe we're talking about different things :P)

[indigo veldt]

I don't think closures would work yet either, but they could take arguments, just have to be top-level funcs. I think we are probably imagining the same thing long-term, but I'm just thinking through the short term restrictions 🙂

[vestal stream]

@indigo veldt here's a quick mockup if what I had in mind re: registering containers
https://gist.github.com/vito/997da4ffd3f863451b0bd67560ae9ec3#file-go-test-container-go-L71-L85

so Dagger would run the Container 'lazily', and understand nonzero exit code == failed, and cache it, with anything else being an error

it's less flexible than funcs or just registering results willy-nilly, but it's nice that you can just run those containers context-free, and it's easy to support things like hopping into the check's container, etc.

[indigo veldt]

Yeah I can see the paths to getting the func registration to work for subchecks but it's fraught right now because so much of the code is in need of cleanup, so I'm just gonna save that for the future when we are actually merging things and have less migraine-inducing code to work with 🙂

I do like the WithContainer idea too, let me see if that's more plausible in the short term

[vestal stream]

I'm starting to understand the current approach more - looking at your other example, it worked by making a codegen'd SDK call to a Check that returns *EnvironmentCheckResult, which is pretty cool, and I think I can implement this using that approach, the only awkward thing is it's a call back to the current environment, meaning you need to codegen partway through implementation, but that doesn't work if your code doesn't compile 😅

I guess you can also call a Function that returns an *EnvironmentCheckResult?

[indigo veldt]

Yeah exactly I'm trying to avoid the need to generate "self-bindings" at all costs, we went down that road w/ cloak and ran into exactly the problem you mentioned.

I guess you can also call a Function that returns an *EnvironmentCheckResult?
That's what I was trying, and it's totally possible. If you look at the main funcs that call .WithCheck_, that's exactly what's happening. There's just a few wrenches thrown in because .WithSubcheck_ in this case is being called in a resolver itself. The Go SDK server code would need some updates to handle that, and while possible I just don't think it's worth it yet...

The WithContainer approach seems feasible so far though, working on it

[indigo veldt]

Seems to be working 🎉 : https://github.com/dagger/dagger/pull/5443/commits/66035856881be257d0424ce930bff2c2f1c03343

I can't tell yet if this is something we'll want in the api the long term, it's definitely convenient now and not hard to support though, so that's a pretty convincing argument to keep it 😁

[vestal stream]

rad! I'll try it out

yeah, my gut feeling is that this will be really helpful in a lot of cases, and I'm not seeing much downside to including it

[indigo veldt]

The only route I see where we wouldn't include it is if A) we get the func approach working (which we should strive for either way) and B) that approach lets you wrap a Container exec call and is so easy/seamless that the withContainer API would be redundant. Otherwise, totally, we should keep it.

[vestal stream]

@indigo veldt

❯ with-dev ../../bin/dagger checks
[45.2s] check unitTest
FAIL unit-test
[49.2s] check integTest
PASS integ-test
Hello, 10.87.0.11:38906!
[0.57s] exec go test -run ^TestGetMsg$ github.com/dagger/dagger/universe/_demo/server/cmd/server
ok      github.com/dagger/dagger/universe/_demo/server/cmd/server   0.005s
[0.17s] ERROR exec false
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
WARNING: Using development engine; skipping version compatibility check.
• Engine: dffa845689ab (version devel ())

I'm using the development engine, btw.

[indigo veldt]

Oh I committed the code I was using to test WithContainer that purposely fails lol https://github.com/sipsma/dagger/blob/66035856881be257d0424ce930bff2c2f1c03343/universe/_demo/main.go#L41-L41

Just delete those lines

[vestal stream]

it works though! just pushed

[indigo veldt]

Ohhh! I see what you're saying! Nice!!

[indigo veldt]

Also btw if you didn't know you're using a development engine and version compatibility checks are disabled, just fyi

[vestal stream]

thanks, I'll look into that!

[vestal stream]

@indigo veldt are sub (maybe sub-sub) checks meant to be shown in dagger checks? 🤔 not seeing'em

[indigo veldt]

I was just typing a message to say that I just pushed a fix to display those correctly 🙂 Though actually, I didn't do it recursively yet, just one level deep

[vestal stream]

lol, nice

much better
I also had a local change to silence the version check that I was about to push 😄

[indigo veldt]

Yeah the output still needs some love but it's semi-comprehensible now. Would love it if someday the tui showed the full list of checks with spinners next to them as they ran and all that good stuff

[vestal stream]

totally, was just thinking maybe this is time to add progrock support for trees in addition to graphs, or something like that

[indigo veldt]

Random thought tangential off the withContainer api on environment check, an environment is in the implementation literally just a container, each resolver invoked via a with exec, so I'm wonder if we should just embrace that in the implementation (not the user facing api)... All that code has turned into extreme spaghetti, I have a suspicion just modeling everything as a container op might clear a lot of that up. Also opens interesting possibilities like changes made to the container filesystem being persisted across steps in a pipeline that uses the environment's resolvers, or making it easy to programatically change the execution environment via the normal container API...

[vestal stream]

Seems like a worthwhile experiment (modulo demo readiness)! Containers definitely have a lot of valuable properties

[indigo veldt]

I think the most valuable property might be that they are serializable to an ID. But since all graphql resolver executions involving an environment are just container execs, that means arbitrary queries to those environments are all magically serializable to an ID. It's vaguely similar to our conversation a while back about using queries to construct IDs, except this would only work for environments atm.

And yes this is very much an idea for later when we are in the process of actually merging all this stuff, don't care for the demo 🙂

[vestal stream]

Yeah exactly. This was why Bass/Bass Loop was so thunk-centric; by using a serializable format all of a sudden you (or a fancy web UI) could take those "serializable closures" and run them anywhere, anytime. If you see a CI build fail you could literally copy-paste the thunk JSON (https://loop.bass-lang.org/thunks/S4VPGVBMJR11I) and pipe it to bass --run to reproduce it. Our IDs are less self-contained, and involve pesky things like local dirs more often, but the same properties hold while the session is active at least.

[vestal stream]

going to see if there's any low-hanging fruit here; it seems like we might be able to just use Progrock already in one way or another, maybe using Groups to represent checks or something, even though it's not a perfect match. it'd just be a lot easier than trying to integrate a tree and a graph in the same TUI

[vestal stream]

@indigo veldt re: check names and field names (saw your comment), a description would definitely be nice if that's not already on your queue 🙂

I'm finding myself wanting to put descriptive values like go test [packages...] there so it shows up in the UI, it didn't even occur to me that they end up being used as field names

actually I can probably just take a crack at it while I work on the rest of this, seems straightforward

but wanted to relay the field name feedback anyway

I'm guessing that's related to their usual practice (before Container support) of referring to a method name (ish) that gets exposed in the schema

(meta: I should probably stop using this as a general purpose thread, oops)