#Time taken to run dagger module CI

1 messages · Page 1 of 1 (latest)

shut shoal
#

I am doing something similar with my modules. Although we don't have a whole lot of tests and we have ephemeral storage so, no cache for initial ops. Timing checks out. It obviously takes longer for me but not much.

ashen mason
#

Yeah one of the big reasons I want to optimise this is to encourage people to run their checks locally when working on the mono repo. At the moment the CI beats some of our local machines! All modules in our mono repo are in fact dependencies of the main module. This is because we run their module tests, so they are basically part of the dependency chain one way or another. The thought I am having is, why run dagger develop when dagger has already codegened every module in the dependency chain once? Simply reuse this codegen... Not sure how simple / hard this is, but its certainly the simplest solution to me haha

#

Right now, we run dagger develop over each module by doing:

  1. get src directory of mono repo
  2. set workdir to module directory
  3. run dagger develop

Implicitly this results in poor caching because any change to the monorepo invalidates the cache. We could be more selective about the directory:

  1. get the directory of the module
  2. run dagger develop

However in cases where modules import each other, the code generation would fail. Therefore in order to do this, we would require a bit more smartness. Include module directories that are in use only. This is a little harder to do though.

ashen mason
#

You can test it using some hacky non-sdk ways to call the engine

#

This PR exposes the Module from the CurrentModule.

#

By using it, we could call GeneratedContextDirectory() on it and on dependencies as well. There was no performance hit for doing so, and the previously generated codegen (from module load) was returned .

#

@fallen iris if my colleague and I brushed this up with some tests and other things, could we see this getting through by any chance ?

fallen iris
#

cc @turbid python @shrewd glacier @amber hollow wdyt? 👆

ashen mason
#

FYI @warm pumice here is the PR

shrewd glacier
#

Just to be sure to understand correctly. You are running dagger develop on every module because you want to be able to run go test on them, right? (and so you need the generated code to be there).
And because it's too slow, you'd like to get access to the GeneratedContextDirectory() directly, right? How is used this result?
(sorry I have the feeling I'm missing something)

#

FYI we are currently looking at improving the SDK interface in a way that would (at least that's the goal) improve the performances of things like dagger develop. Basically by splitting the different phases and rely in a better way on caching so we invalidate it less often, especially while working on a module code (for instance if you don't change the dependencies, the internal directory will never change, if you don't change your signatures, dagger.gen.go will never change, etc).
I didn't thought about exposing those functions at the level of CurrentModule but that might be a good idea when you want to access more of the internals.
But I'd like to understand a bit better the underlying issue.

turbid python
#

we try and take care that even if ModuleSource changes (e.g. building different commit, etc), then we won't re-run the entire thing (with global function caching in the works with @thin hawk)
but Module points to ModuleSource
so you end up in a scenario where potentially getting certain stuff from CurrentModule is gonna cache and give you results you weren't expecting

#

ideally the solution to this is to be able to mark certain methods like this (and other ones, like Cloud.traceURL), and then modify the cache key after the fact so we get the correct behavior

#

that said, this doesn't sound like a blocker to me

amber hollow
#

On an API pov, I would just expose GeneratedContextDirectory at dag.CurrentModule().GeneratedContextDirectory() instead of returning a ref to the module directly, since we already got dag.CurrentModule().Source()

#

But I'm not sure why dag.CurrentModuel().Source() isn't enough

shut shoal
amber hollow
#

So exposing directly GeneratedContextDirectory would just be a faster way to get the generated code?

turbid python
amber hollow
#

Yeah that make sense

ashen mason
#

Hi guys. The reason why I am so keen on exposing module is because we have a dagger modules mono repo. What I want to do is run the codegen for all the modules in this repo and then stitch it together - so we can run things like go test .

Our mono repo has a modules folder containing all modules. The CI for this repo is another dagger module living in infrastructure/dagger. Every module has another dagger module inside called tests which depends on the module. Our CI module depends on all the tests modules so it can call their .All function to run all the tests. In this way, every module is in some way a dependency of the CI module.

| - modules
    | - helmfilemodule
        | - tests
    | - terraformodule
        | - tests
    | ...
    | - another module
        | - tests
| - dagger.json
| - infrastructure/dagger
    | main.go
#

One of the things we were able to achieve yesterday was this:

func (m *Main) withCodegen(ctx context.Context, src *dagger.Directory) (*dagger.Directory, error) {
        q := querybuilder.Query().Client(dag.GraphQLClient())

    var id dagger.ModuleID
    err := q.Select("currentModule").Select("module").Select("id").Bind(&id).Execute(ctx)
    if err != nil {
        return nil, err
    }

    currentModule := dag.LoadModuleFromID(id)

    src, err = mergeRecursive(ctx, *currentModule, src)
    if err != nil {
        return nil, err
    }

      return src, nil
}
func mergeRecursive(ctx context.Context, mod dagger.Module, src *dagger.Directory) (*dagger.Directory, error) {
    deps, err := mod.Dependencies(ctx)
    if err != nil {
        return nil, err
    }

    // entries for our module 1 generated
    for _, dep := range deps {
        src = src.WithDirectory(".", dep.GeneratedContextDirectory())

        src, err = mergeRecursive(ctx, dep, src)
        if err != nil {
            return nil, err
        }

    }

    return src, nil
}

...essentially we were able to stitch together all the codegen for our monorepo - without incurring a cost

#

This is essentially our ultimate goal. I am not heavily tied to the approach that @warm pumice and I came up with yesterday, and if there are different ways to achieve the same thing it would be great.

ashen mason
# amber hollow I'm not sure I understand that part, you can already do: `dag.CurrentModule().So...

I did try this approach at first. However there is some weirdness with it. Firstly .Source() returns the directory where the module source lives. For our CI module this is the infrastructure/dagger directory, which doesn't include the dagger.json file. I guess I would have to recompose the original directory structure... which could be done. However I believe this would result in all our modules + dependencies being recodegened as soon as you call .AsModule due to the fact that this isn't the exact set of steps which dagger uses to load your module - I don't think the cache matches. I might try it to confirm but that was my initial suspicion.

amber hollow
ashen mason
#

Happy to go this route

amber hollow
#

I’m happy to review the PR tomorrow, if you can add integration tests that would be nice 🙂
if you need help feel free to ping me on your PR 😉

ashen mason
#

Cheers Vasek! Yep will have a crack. If I get stuck, will ping you

ashen mason
#

Hi Vasek I am up to writing integration tests. This is where I will need your help. I believe I would need to make an integration test in core/integration for the CurrentModule type. A few questions about this:

  1. Should one test written for the go SDK suffice ?
  2. Which test suite should it slot into? I was thinking module_config...
  3. I noticed tests in module_config and module_call construct an SDK called CoolSdk a lot of the time. What is the significance of this pattern?
  4. Would the test need to check that the values returned by GeneratedContextDirectory and Dependencies matched some expected output? Or is just testing they don't panic enough?

Thanks in advance!!!

amber hollow
# ashen mason Hi Vasek I am up to writing integration tests. This is where I will need your he...

Heyyy, you simply need to add your 2 tests here: https://github.com/dagger/dagger/blob/4d6b3e4897267f78898c421c4a6d2cf7f4ebc6d0/core/integration/module_test.go#L1883

We already have a bunch of test for the CurrentModuleAPI, so you can just your small test here.
You do not need to do something super fancy, the reference module is in go so for GeneratedContextDirectory, you could verify that the directory contains dagger.gen.geo

Here's an example of test case you could write (based on "name": https://github.com/dagger/dagger/blob/4d6b3e4897267f78898c421c4a6d2cf7f4ebc6d0/core/integration/module_test.go#L1884)

        c := connect(ctx, t)

        out, err := c.Container().From(golangImage).
            WithMountedFile(testCLIBinPath, daggerCliFile(t, c)).
            WithWorkdir("/work").
            With(daggerExec("init", "--source=.", "--name=test", "--sdk=go")).
            WithNewFile("/work/main.go", `package main

            import (
              "context"
              "dagger/test/internal/dagger"
            )

            type Test struct {}

            func (t *Test) Fn(ctx context.Context) (*dagger.Directory, error) {
                return dag.CurrentModule().GeneratedContextdirectory(ctx)
            }
            `,
            ).
            With(daggerCall("fn", "entries")).
            Stdout(ctx)
        require.NoError(t, err)
        require.Contains(t, out, ["dagger.gen.go"])

For Dependencies, you can do the same, you can return a []string of the dependency name for example and check that it matches the expected

ashen mason
#

Awesome! Thank you Vasek will have a try!

ashen mason
#

Thanks Vasek I was able to get my integration tests to pass. Got one for each function. Was just wondering is there any problem if I squash my commits prior to merge ?

fallen iris
amber hollow
ashen mason
#

Hi Vasek. I will squash my commits and fix the DCO issue

#

Then give you the go ahead

ashen mason
#

Hi Vasek. Ready for review

#

Is this because my commit has a long body ?

#

Happy to remove the body if you don't deem it necessary

#

Hmm doing some testing, I have just realised its not caching as nicely as when I first tested this

#

I believe this is because a different dag is produced by doing moduleSource.GeneratedContextDirectory() vs moduleSource.AsModule().GeneratedContextDirectory()...'

#

will just have a look into this

#

I think moduleSource.AsModule() is what the module load is doing...

ashen mason
#

Hmm that's not it

#

If I compare the dags produced by the initializing module step and the currentModule.generatedContextDirectory():

initializing module:

image.png
#

currentModule.generatedContextDirectory()

image.png
#

expanding that Container before the .withoutDefaultArgs can see this:

image.png
#

so the difference is the absense of the .withUnixSocket and .withEnvVariable steps

ashen mason
#

Figured out whats wrong. I was testing before using dagger call dev which ran dagger in dagger. This didn't mount any of my host SSH setup inside the container which does the codegen build. When testing by running the engine directly on my host, dagger now picks up my SSH setup. This accounts for the difference being seen between the "load module" dag and the currentModule.withGeneratedContext()dag

ashen mason
#

So I can see that the difference comes from this code in the go sdk code generation...

        clientMetadata, err := engine.ClientMetadataFromContext(ctx)
    if err != nil {
        return nil, nil, fmt.Errorf("failed to get client metadata from context: %w", err)
    }

    if clientMetadata.SSHAuthSocketPath == "" {
        return nil, nil, nil
    }
#

the client metadata context is different for when you call .GeneratedContextDirectory()` from within a module vs from when its called to initially load the module.

#

This results in a different dag and not hitting the cache unfortunately!

#

We end up codegenning again...

#

Any ideas on how we could ensure we get a cache hit here ?

fallen iris
#

I will copy @opal breach because he has been quite involved with the SSH auth part

ashen mason
#

I did some testing where I modded the CLI to call GeneratedContextDirectory before serving the module. I found that I got a cache hit as a result of this and it worked.

#

GeneratedContextDirectory appears not to be used when loading a module. I think .AsModule() seems to take care of loading codegen some other way somehow

fallen iris
#

I wonder if we suffer from this in our own CI? we also do a programmatic "dagger develop"

ashen mason
#

I'm about to go on leave again at work btw guys. So I will leave this in the capable hands of @warm pumice

ashen mason
#

With the release of genearlised function caching, perhaps my code will now work ?

ashen mason
#

Well just rebased and tested and still dont get a cache hit.

fallen iris
#

(to avoid breaking changes)

#

if you dagger init a new module you should see an extra field in the dagger.json

ashen mason
#

ah so perhaps it was a bit of a stretch but I wondered if that function caching feature applied to internal functions

#

the function in question here is GeneratedContextDirectory

#

which is an internal function..

#

all good

#

@amber hollow this has been rebased now

#

Still not doing what I wanted it to though

#

Its not working for the same reasons as outlined above... the Go SDK includes a mounted SSH socket when that socket is available in the current context. This results in a different dag.

#

Oh well. for now I guess this one is on the back burner.

fallen iris
ashen mason
#

I was thinking one way to potentially get the generated code out without incurring this problem, would be if the function GeneratedContextDirectory just retrieved a previously evaluated dagger.Directory rather than recreating the entire dag

#

This would require the dag to be created ahead of time

amber hollow
ashen mason
#

Awesome

amber hollow
#

@ashen mason Thanks for your PR, I reran the CI and verified your test, it's all good 😉
I can see in the CI that the cache hits (see my comments)
The PR is merged so you should find these 2 news methods in the next release.

ashen mason
#

Amazing 🙂 Thank you Vasek!

#

This will help us move away from running dagger develop via dagger in dagger, so this is still a win for us