SDK: add moduleTypeDefs to module SDK, to register type definition
#10584
Conversation
d4d86b4 to
943fd78
Compare
| TypeDefs( | ||
| context.Context, | ||
|
|
||
| // Current module dependencies. | ||
| *ModDeps, | ||
|
|
||
| // Current instance of the module source. | ||
| dagql.Instance[*ModuleSource], | ||
| ) (*Container, error) |
There was a problem hiding this comment.
Should we not attempt to try and have this directly return a list of TypeDefs, or even a Module?
Even if the SDK runtime chooses to use a container behind the scenes, I think it makes sense to allow an SDK to directly return them if possible.
There was a problem hiding this comment.
This was with the idea to keep the changes as small as possible, but yes it might make more sense to do that. I'll have a look.
There was a problem hiding this comment.
I'm trying to see what would be the best option to do that, looking at both the Go SDK and the Java as they are quite different.
So I wonder if it wouldn't be a good idea to add something like a fromJSON or load function on the dagger.Module type that will take a json representation of those types.
So we can dag.Module().FromJSON(jsonTypesFile) and the runtime code is very small and almost shareable between SDKs, no function execution required as currently.
In Java SDK that could be something like:
func (m *JavaSdk) ModuleTypeDefs(
ctx context.Context,
modSource *dagger.ModuleSource,
introspectionJSON *dagger.File,
) (*dagger.Module, error) {
mvnCtr, err := m.codegenBase(ctx, modSource, introspectionJSON)
if err != nil {
return nil, err
}
// returns a json file with the type details
typesJson, err := m.generateTypes(ctx, mvnCtr, introspectionJSON)
return dag.Module().
FromJSON(typesJson), nil
}The main change is the SDK needs to generate this json file instead of the type registration code.
Basically this is just this code but in JSON:
return dag.Module().
WithObject(
dag.TypeDef().WithObject("MyModule", dagger.TypeDefWithObjectOpts{SourceMap: dag.SourceMap("main.go", 8, 6)}).
WithFunction(
dag.Function("ContainerEcho",
dag.TypeDef().WithObject("Container")).
WithSourceMap(dag.SourceMap("main.go", 10, 1)).
WithArg("stringArg", dag.TypeDef().WithKind(dagger.TypeDefKindStringKind), dagger.FunctionWithArgOpts{SourceMap: dag.SourceMap("main.go", 10, 34)})).
WithFunction(
dag.Function("Hello",
dag.TypeDef().WithKind(dagger.TypeDefKindStringKind)).
WithSourceMap(dag.SourceMap("main.go", 14, 1))))@jedevc is that the kind of thing you had in mind?
There was a problem hiding this comment.
Yes! This is pretty much exactly what I had in mind as the end state. To simplify getting to that state, we could even have dag.Module().AsJSON(), so we keep similar typedef code for now and transfer it later? Just thinking out loud.
But yeah, in the spirit of keeping changes minimal for now, we only need to change the TypeDefs interface function to return a Module - under the hood, we can still do the changes in this patch, where we're dispatching that to a Container.
We can come and do the big JSON changes for each SDK one-by-one later, as a performance optimization.
2679ef5 to
479970e
Compare
Go SDK now has a moduleTypeDefs method that is dedicated to return types exposed by the module. The `moduleTypeDefs` is similar to `moduleRuntime` but the generated entrypoint only contains the types and not the invocation code. Codegen contains a new flag `--typedefs-only` to only generate the part related to types. Other than this flag, one change in the codegen is a `register` function is created to handle the management of the types. `invoke` function will call `register` when the parent name is empty. And basically when `--typedefs-only` is set, the invoke function is not created but the `register` is and is called instead of `invoke`. Signed-off-by: Yves Brissaud <gh@lgtd.io>
This is an optional new entry point on the module SDK. It's designed to perform the type definition from the module like the actual moduleRuntime when an empty string is passed as argument. If the function doesn't exist, the moduleRuntime is used instead. But this allows to decouple both of them so we can registry types of a module without the runnable module itself because to build it needs the types defined by the module Signed-off-by: Yves Brissaud <gh@lgtd.io>
Use `moduleTypeDefs` instead of `moduleRuntime` if available. Cache the runtime as expected if `moduleTypeDefs` has been called. Signed-off-by: Yves Brissaud <gh@lgtd.io>
3fb3896 to
f7d341d
Compare
Signed-off-by: Yves Brissaud <gh@lgtd.io>
|
@eunomie Looked at the I think the fundamental problem is that we end up running codegen twice now in this new scheme. That's not ideal for performance besides the socket issue it introduces. I think splitting out the new I haven't parsed through the exact behavior changes |
|
Thanks @sipsma for the explanation.
The goal is to run codegen twice, and to decouple typedefs from runtime. A very simple example would be the following: func (m *MyModule) ContainerEcho(stringArg string) *dagger.Container {
return dag.Container().From("alpine:latest").WithExec([]string{"echo", stringArg})
}
func (m *MyModule) Echo(stringArg string) *dagger.Container {
return dag.MyModule().ContainerEcho(stringArg)
}The
All that means 2 codegen phases, one for typedefs, one for function calls. Right now it's the same with just a flag, that could be completely different, or use something like @jedevc is mentioning above, but my understanding is it will not really change a lot to this problem, no? So yes, for this very first phase it would be possible to avoid the different codegen and only use the existing |
|
Regarding performances, yes there's a cost here. But I don't see a lot of different ways to avoid it. Thinking again, maybe the idea of an intermediate json might help has there's not anymore this typedefs container. Maybe. |
Could you detail a bit more? I'm not sure to understand the link between the container cached and not evaluated and the SSH socket. |
Hah, okay, I chatted with @eunomie, lemme try and summarize and explain. So, this is part of the work to do self-calls. We need the separation into two phases for this. For the go sdk (as currently structured in Here's why:
But. In the scenario of self-calls the code won't be valid after generating all the types and methods - because we'll be missing the self-bindings (we only know what those types should be after step 3). So we can't build it, so we can't get the typedefs. We have this weird circular dependency. Thankfully, we can fix this. So, we need these two phases. To make sure that we fix the caching issue that @sipsma points out, we need to eagerly evaluate the However, as @sipsma points out, there's a little bit of a perf issue here (curious to see how this pans out though, it might not be terrible?). For every module load, we're now having to generate, build, and execute two binaries. Ideally, the solution to this is to improve |
|
This is not a solution at all, but I just confirmed that with this change, that works: diff --git a/core/sdk/go_sdk.go b/core/sdk/go_sdk.go
index 3d6fc8390..664aff2e9 100644
--- a/core/sdk/go_sdk.go
+++ b/core/sdk/go_sdk.go
@@ -523,13 +523,13 @@ func (sdk *goSDK) baseWithCodegen(
// cause failures if we also mount this in Runtime.
// Issue we run into is that when we try to run sdk checks
// using .dagger, it fails trying to find the socket
- sshAuthSelectors, err := sdk.getUnixSocketSelector(ctx)
- if err != nil {
- return ctr, err
- }
- if len(sshAuthSelectors) > 0 {
- selectors = append(selectors, sshAuthSelectors...)
- }
+ //sshAuthSelectors, err := sdk.getUnixSocketSelector(ctx)
+ //if err != nil {
+ // return ctr, err
+ //}
+ //if len(sshAuthSelectors) > 0 {
+ // selectors = append(selectors, sshAuthSelectors...)
+ //}
// now that we are done with gitconfig and injecting env
// variables, we can run the codegen command.(if there's no need for a private repo) My understanding is the first codegen part (the one generating the typedefs) shouldn't require the ssh socket, as it shouldn't need the dependencies if I'm right (functions can return a local type or a core type, but not a dependent type, right?) |
Note
This has been extracted and cleaned from #10475
This is a first step to #10336 and #8030
This PR introduces a new
moduleTypeDefsmethod modules can implement. This method acts likemoduleRuntimeand should returned a container. Currently the container returned bymoduleRuntimeis called in two different ways:In Go for instance, this is done in the generated
invokefunctions with a code like the following:The idea of
moduleTypeDefsis to split those two responsibilities.:moduleTypeDefsmoduleRuntimewill be executedNote
For compatibility reasons and the time to transition all modules to this new behaviour, if the
moduleTypeDefsfunction is not exposed by the module SDK, then themoduleRuntimewill be run and the container executed as before.Go SDK
To simplify the management of those cases, the generated code responsible for the type registration (what was inside the
case "") has been extracted into a specificregistermethod.The behaviour of the container returned by
moduleRuntimeis the same.But the generated code for the container returned by
moduleTypeDefsonly contains theregisterfunction, not theinvokefunction. This allows to decouple the type definition from their invocation and will help to build self invocation without circular dependency issues.Other SDKs
Other SDKs will follow in different pull requests to keep them easier to review. The Java one is almost ready and will follow.
Next
With the type definition decoupled from the invocation, it will be possible to inject the module types in input of the module to generate types against the module itself.