-
Couldn't load subscription status.
- Fork 132
Fix models support with compose include #824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be necessary, as next lines merge equivalent mapping syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to remove the comment only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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):