#Check type: dual purpose?

1 messages · Page 1 of 1 (latest)

lavish loom
#

🧵

tardy lodge
#

Yes, in practice if you change the name of a check after getting the result, I guess that's weird but doesn't hurt much. But I agree it's odd and could probably avoid it in the API

#

By just making those parameters immutable after the check has been initialized

lavish loom
#

I'm trying to reconcile this with the API I know in dev branch

tardy lodge
lavish loom
#

Does the SDK still let me register an arbitrary function as the backend implemention of a check? Or does it not work that way anymore?

tardy lodge
# lavish loom Does the SDK still let me register an arbitrary function as the backend implemen...

Yes that's how it works, though that's a privilege only afforded to SDKs running in environments, doesn't work for "outside clients" not running in an env.

eg. this: https://github.com/dagger/dagger/pull/5378/files#diff-42fb89a456403754fd2e35d03ea1eff6f1f90a281450e3a10bad517208bd2b69R31-R35
That in particular isn't the API we present, but we could do that (we accept named funcs in Go, in python we use the decorator approach). That wouldn't affect the graphql API though since obviously go functions aren't a graphql type. Every SDK has to do it's own little magic to present that interface to users

lavish loom
#

So in the current checks PR branch, what would a Go snippet look like of adding a check to my env?

#

I'm having trouble picturing it at the moment

tardy lodge
#

Basically, that API that's used by env code to register functions is by it's very nature a facade since like I said a go func can't be serialized into a graphql request.

#

The graphql API you see in the schema is the real one underneath it. The SDKs register their checks by name and also keep track of which func maps to which name (in python, this is with decorators, but same idea).

Then when a check needs to be invoked, the engine invokes the entrypoint with data that basically maps to "give me the output of the entrypoint with this name".

lavish loom
#

so when I call WithCheck in my sdk, what gql query does the sdk make?

tardy lodge
#

so basically dag.CurrentEnvironment().WithCheck(dag.Check().WithName("theNameOfTheFunc").WithDescription("the //comment above the func"))

#

Oh one thing I forgot to mention, I'm including withCheck under a separate section in the graphql schema where everything is doc'd as "internal-only". The idea there is that we can add some mechanisms that let internal-only apis be treated special so they e.g. only show up in env sdk code, don't show up in our public graphql docs, etc.

lavish loom
#

so if I understand correctly, my function doesn’t actually implement the check anymore (ie it’s not a resolver to return my check’s result) it’s more of an init function that sets up a container, and later the resolver for my checks result will just wrap exec of that container

#

“anymore” -> compared to my imaginary DX example from early design branch, so arguably it may never have worked any other way 🙂

tardy lodge
# lavish loom so if I understand correctly, my function doesn’t actually implement the check a...

Sorta but it's more than that, we support multiple possible return types for the implementation of a WithCheck entrypoint; that's what this enum is all about: https://github.com/dagger/dagger/blob/4fbe60210e1b2cb44c649dac7bd32ea432ed3e58/core/schema/env.graphqls#L190-L195

So an entrypoint is allowed to return a CheckResult, which is the most obvious implementation, but it can also return a string (in which case the string becomes the check output and success is based on exit code) and it can even return a Check itself (which results in recursive evaluation, useful for dynamically constructing Checks from Subchecks).

Previously, we supported all this behavior but it was a huge mess because the contract between engine+sdk was implicit and it was very hard to reason about the caching behavior we needed in each different scenario.

So I updated everything to encode all that in the graphql API as much as possible. Now we pretty much just ask SDKs to tell the engine what checks exist and what their return type is, plus we ask them to exit 0 for success, 1 for "user-code" error and 2 for "internal error".

After that, then engine takes care of everything else.

#

There's more considerations involved in the design of this too around laziness, but I'll spare you unless it's helpful 🙂

lavish loom
#

So then once a check is registered with the engine via CurrentEnvironment().WithCheck(), what does the engine do with that information?

#

It exposes those checks in the frontend API, and routes resolver requests for that check to the SDK, via the hidden wire protocol?

tardy lodge
lavish loom
#

like a mini-rpc over graphql 🙂

tardy lodge
lavish loom
#

I'll be honest my kneejerk reaction is to be overwhelmed

#

It's quite a beast to understand

tardy lodge
lavish loom
#

Yeah that does give me hope

#
  • WithCheck(EngineTests) -> ah, EngineTests is a check
  • func EngineTests(ctx context.Context) (*Check, error) -> I guess not, EngineTests is a function that returns a check. So what's a check?
  • return dag.Check().WithContainer(testCtr), nil -> looks like a check is a wrapper around a container
  • mmm but Erik tells me it can be much more
  • Can a check be a function?
  • But not the same kind of function as EngineTests?
  • -> ok I'm lost
tardy lodge
#

Oh the Check().WithContainer() thing? That's leftover from before I had fixed all this stuff. It should probably just be returning (string, error) now. I'll fix that once I'm done with the current stuff I'm doing.

But yeah that's not the best example to lead with because it's literally just one Check. Honestly the integ test env I linked to is probably more helpful: https://github.com/dagger/dagger/blob/4fbe60210e1b2cb44c649dac7bd32ea432ed3e58/core/integration/testdata/environments/go/basic/explicitdep/main.go#L10-L17

lavish loom
#

Ok checking that out now

#

what's a StaticCheckResult ?

tardy lodge
tardy lodge
#

Like when your entrypoint code is actually implementing the check itself and you just need to return the result, you use that

#

I agree btw that dag.Check().WithContainer() is confusing until you fully grok it, but that's also only meant for advanced cases. We added that when @rotund ledge was implementing the go env in universe where we are dynamically creating a check based on parsing out go test packages, which most users will never have to think about.

#

Like it's great to have, but it wouldn't be in our docs on "Getting Started With Checks" probably

lavish loom
#

I'm still confused but trying to understand why

#

Sorry don't mean to be a downer

tardy lodge
#

Overall, the goal of everything here is to allow users to write code implementing a Check as intuitively as possible. I actually think we've made good progress on that, but I can see how it's not clear when the examples I have to show you are dagger's own CI (which I just haven't simplified yet) and our internal integ tests (which are just not a good real-world example)

lavish loom
#

Why do I need to construct a CheckResult with queries to the API? Couldn't I just return the values?

#

Feels like calling a MakeInt(42) system call just to return an int in my ƒunction, if you see what I mean

tardy lodge
lavish loom
#

func foo() int { return os.MakeInt(42) } 🙂

tardy lodge
#

and the performance downsides are extremely minimal thanks to the fact that the graphql server is in the engine container now; those requests take microseconds

#

(I don't have data to back that up, but I'd put money on it 😁 ; I can say in practice that it's not noticeable)

#

Wait, actually, scratch that, it doesn't even make a request at all, it just lazily puts that data into the querybuilder: https://github.com/dagger/dagger/blob/4fbe60210e1b2cb44c649dac7bd32ea432ed3e58/core/integration/testdata/environments/go/basic/dagger.gen.go#L3786-L3801

So there's actually no meaningful overhead to it. We get the benefit of the fact that every SDK is in full agreement on how to construct a CheckResult from static params and consistency in how those would be returned, but we don't need to do an API round trip.

#

So it's not os.MakeInt(42), it's just 42

lavish loom
#

But what happens if instead of this:

func SadStaticCheck(ctx context.Context) (*CheckResult, error) {
    return dag.StaticCheckResult(false, StaticCheckResultOpts{
        Output: checkOutput("SadStaticCheck"),
    }), nil
}

I do this:

func SadStaticCheck(ctx context.Context) (*CheckResult, error) {
    return &CheckResult{
          Result: false,
          Output: "SadStaticCheck",
    }, nil
}

?

Because that is way less confusing to me

tardy lodge
lavish loom
tardy lodge
#

I totally agree I like how the second one looks better, and actually Helder has an issue about improving how our SDK codegen handles "simple objects" which would probably entail simplifications like that: https://github.com/dagger/dagger/issues/4920

#

So, as usual, @empty coral has already thought about it and has ideas on how to improve it 😄

lavish loom
#

Of course 🙂

#

I have to go pick up the kids. Will digest all the above, thanks for walking me through it.

#

In the back of my mind is the feedback we got this week, that is steering us towards exposing raw functions more directly. That seems to be what people are most excited about. Not sure yet what that means for this branch

#

It feels like an opportunity to narrow down the scope of the first thing we merge - for example the concepts of "current entrypoint", "entrypoint return value" in the mini-rpc, that kind of looks like a function invocation to me. Maybe focusing 100% on functions first, would tidy that up?

#

But maybe that's a bad idea

#

Also I still need to make the orthogonal proposals to change terminology, and in particular the meaning of the word "env"

#

almost completely orthogonal I think

#

The big change for me, is that I always assumed raw functions (as we now call them) would not be interesting on their own to end users. But I'm starting to get a sense that that was wrong

tardy lodge
#

I've been planning on Functions being the follow up to this one. I chose checks first because they actually resulted in something invokable from the the CLI (though obviously we have been talking about functions being invokable from the CLI too now)

for example the concepts of "current entrypoint", "entrypoint return value" in the mini-rpc, that kind of looks like a function invocation to me
Yes, that's 100% true and a result of the fact that underneath the hood everything is a function. That whole minirpc is going to be re-used by both Function and every other entrypoint type.

That's why it was so useful to explore the whole landscape first w/ the Zenith branch, it's now possible to do an implementation that is both clean and ready for everything else that's coming up.

#

So personally, I'm not especially worried about starting with Checks and then doing Functions next. The practical consideration here too is that I think it's worth merging something that we have a reasonable threshold of confidence in, just to alleviate the pain of eternally being on branches.

#

And the fact that even once we merge, it's all hidden and thus we are free to tweak anything however we want gives us lots of safety