Fix models support with compose include#824
Fix models support with compose include#824doringeman wants to merge 3 commits intocompose-spec:mainfrom
Conversation
Networks schema allows null but models schema requires objects. Fixes validation error when using models short syntax with includes. Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
ndeloof
left a comment
ndeloof
left a comment
There was a problem hiding this comment.
Changes to transformStringSliceToMap are not required, this same mechanism is used in many other places without any issue.
| var mergeSpecials = map[tree.Path]merger{} | ||
|
|
||
| func init() { | ||
| mergeSpecials["models.*.runtime_flags"] = override |
There was a problem hiding this comment.
This requires an update on the compose-spec documentation to make it clear runtime_flags won't be appended
see https://github.com/compose-spec/compose-spec/blob/main/13-merge.md#exceptions
There was a problem hiding this comment.
IMHO you don't need such a rule, for the use-case to fully redefine a model one would rely on !override
| // Check if both sides are string arrays for short syntax only | ||
| if rightArr, ok := c.([]any); ok { | ||
| if leftArr, ok := o.([]any); ok { | ||
| if isStringArray(rightArr) && isStringArray(leftArr) { | ||
| return mergeStringArrays(rightArr, leftArr), nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Otherwise, use map merge for long syntax or mixed syntax |
There was a problem hiding this comment.
this should not be necessary, as next lines merge equivalent mapping syntax
There was a problem hiding this comment.
You mean to remove the comment only?
There was a problem hiding this comment.
no, I mean this all block. Mutating into mapping format then merge is the right way, and should not require any extra processing - we do the same for the many other comparable structs
In this case I'd need to add |
I don't think so. See |
2 participants
Fixes 3 related issues preventing models from working with the include directive:
models: [llama]→{llama: null}violates schema).importResources).E.g.,
Fixes running
docker-compose upwith the rootdocker-compose.ymlin docker/hello-genai#15.Before this PR:
After the second commit (run
go mod edit -replace=github.com/compose-spec/compose-go/v2=github.com/doringeman/compose-go/v2@80d4df69626c3c3ab08f5c3af0870de5900aecde && go mod tidy && make installin https://github.com/docker/compose):After the third commit (run
go mod edit -replace=github.com/compose-spec/compose-go/v2=github.com/doringeman/compose-go/v2@95abc2eef4c7b7d1278a34c2e548ec47a216acc9 && go mod tidy && make installin https://github.com/docker/compose):