feat: model input validation before build and run#3097
Conversation
Validate CLI inputs for `cog predict`, `cog run`, and `cog train` against the model's OpenAPI schema before the expensive build/pull/start steps, so bad inputs fail fast instead of after a full Docker build or container start. - Add pkg/predict/input_validation.go with schema-driven validation that reports friendly errors (unknown/missing inputs, type mismatches, enum values, numeric constraints). - Extract static schema generation into pkg/schema/static so both the image build and CLI preflight share one code path. - predict/train: generate the schema up front from cog.yaml (local source) or the image's openapi_schema label (existing image) and validate before building/starting. Images without the label fall back to the runtime schema fetched after start, preserving prior behavior. - Surface file-read failures with the offending input name. - Add unit and integration tests covering the new validation paths and pin user-facing error wording to guard against kin-openapi changes.
Rename the config-aware schema generator package to pkg/schema/openapi so call sites read openapi.Generate(cfg, dir). Rename the existing renderer file to openapi_spec.go to free the openapi name.
Rename the entry point to openapi.GenerateSchema so it's clear what it produces. In generateLocalOpenAPISchema, rename the loaded document to `spec` (was shadowing `schema`) and the raw bytes to `openapiSchema` (avoids implying JSON Schema).
Address review feedback: use a single var block for the local declarations in cmdPredict/cmdTrain instead of mixing var and := syntax.
Address PR review feedback on input preflight validation: - Coerce '-i name=true' to a boolean and repeated/single '-i name=v' array values to the array's item type, so booleans and numeric/bool arrays reach the runtime with the intended type and pass preflight validation instead of failing as strings. - Preserve typed array elements through Inputs.toMap (previously arrays were always flattened to []string). - Only run preflight validation for existing images when the schema label actually carries the Input/TrainingInput component; otherwise fall back to the runtime schema (guards minimal/malformed labels). - Include the input name in JSON-mode file read errors for parity with the -i path. - Add integration coverage for omitting a required input (local source before build, and existing image before start).
Address remaining PR review feedback: - #7: local-source 'cog predict'/'cog train' now generate the OpenAPI schema once for input preflight and pass the bytes into the build via BuildOptions.OpenAPISchema, so the build reuses them instead of regenerating. This removes the second generation and the chance of drift between the validated schema and the image label. Extracted the schema-selection logic into resolveBuildSchema for unit coverage. - #8: the runtime validates inputs as strict JSON Schema and ignores the OpenAPI 'nullable' keyword, so it rejects an explicit null for every field. kin-openapi honors 'nullable' and would accept it, letting a '--json' request pass preflight only to 422 at runtime. Reject explicit nulls in preflight so it matches runtime. (The other cases the reviewer flagged -- numeric strings and Any/object inputs -- were verified to already match the strict runtime.) Documented the one intentional divergence: unknown inputs are rejected at the CLI rather than stripped as the runtime does.
Rename schemaJSON/schema to openAPISchemaJSON/openAPISchema in the cmdPredict/cmdTrain local-source paths to make the raw-bytes vs parsed distinction explicit and match the BuildOptions.OpenAPISchema field.
Address adversarial review feedback:
- Add tests for the previously untested null-composition branches of
schemaAllowsNullWithoutNullable (oneOf exactly-one, enum null
containment, not) plus oneOf null rejection/acceptance through the
public ValidateInputMapForMode entry point, protecting preflight vs
runtime parity.
- Dedup the two identical quoting loops and four-way message branch in
validateKnownInputs into a quoteJoin helper.
- Harden missingRequiredInput to key off schemaErr.SchemaField
('required') before parsing the free-text reason.
- Document validation ownership across the three entry points and add a
defensive nil guard in NewInputsForMode.
01701f1 to
84d802d
Compare
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds preflight input validation for cog predict, cog run, and cog train against the OpenAPI schema before expensive build/start steps. It also extracts the schema generation into a reusable pkg/schema/openapi package and improves CLI input coercion for booleans and numeric arrays. Overall the changes are well-structured and thoroughly tested.
One minor issue in changed code:
|
Review submitted to PR #3097. I found one minor issue where |
|
LGTM |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds preflight input validation for cog predict/run/train against the OpenAPI schema before expensive build/start steps. The changes are well-structured and thoroughly tested, but I found three issues:
- HIGH:
NewInputsForModesilently drops all inputs when the schema lacks components, causing silent data loss instead of parsing inputs as strings/files. - MEDIUM:
HasInputComponentvalidates the entire OpenAPI document, which can cause false negatives for existing images with structurally incomplete but usable schema labels. - LOW-MEDIUM:
newSingleInputdereferencesresolved.Typewithout a nil check, which can panic on schemas usingoneOfor other compositions where the wrapper has no concrete type.
| // generate+validate it, existing images gate on HasInputComponent), but | ||
| // guard here too so a future caller can't trip a nil-map deref. | ||
| if schema == nil || schema.Components == nil { | ||
| return Inputs{}, nil |
There was a problem hiding this comment.
Returning an empty Inputs map here silently drops all provided values when the schema lacks components. The old behavior parsed inputs as strings/files even without a schema. A future caller that doesn't also run validation will receive empty inputs with no error.
| return Inputs{}, nil | |
| if schema == nil || schema.Components == nil { | |
| input := Inputs{} | |
| for key, vals := range keyVals { | |
| if len(vals) == 1 { | |
| input[key] = newSingleInput(vals[0], nil) | |
| continue | |
| } | |
| anyVals := make([]any, len(vals)) | |
| for i, v := range vals { | |
| anyVals[i] = v | |
| } | |
| input[key] = Input{Array: &anyVals} | |
| } | |
| return input, nil | |
| } |
| // runtime schema for images with minimal or missing labels. | ||
| func HasInputComponent(schema *openapi3.T, isTrain bool) bool { | ||
| _, err := inputComponentForMode(schema, isTrain) | ||
| return err == nil && schema.Validate(context.Background()) == nil |
There was a problem hiding this comment.
schema.Validate checks the entire OpenAPI document (paths, responses, etc.). A structurally incomplete label — e.g., a legacy runtime schema missing paths — can have a perfectly usable Input component yet fail this check, causing an unnecessary fallback to slower runtime validation.
| return err == nil && schema.Validate(context.Background()) == nil | |
| return err == nil |
| switch { | ||
| case resolved.Type.Is("object"): | ||
| raw := json.RawMessage(value) | ||
| return Input{Json: &raw} | ||
| case resolved.Type.Is("array"): | ||
| return newSingleArrayInput(value, schema) |
There was a problem hiding this comment.
resolveSchemaType only handles allOf and anyOf; it does not check oneOf. If a schema uses oneOf (valid OpenAPI 3.0), the function returns the wrapper schema with Type == nil, and these dereferences panic. Guard against nil Type to be safe.
| switch { | |
| case resolved.Type.Is("object"): | |
| raw := json.RawMessage(value) | |
| return Input{Json: &raw} | |
| case resolved.Type.Is("array"): | |
| return newSingleArrayInput(value, schema) | |
| switch { | |
| case resolved.Type != nil && resolved.Type.Is("object"): | |
| raw := json.RawMessage(value) | |
| return Input{Json: &raw} | |
| case resolved.Type != nil && resolved.Type.Is("array"): | |
| return newSingleArrayInput(value, schema) | |
| default: |
|
I'm Bonk, and I've done a quick review of your PR. This PR adds preflight input validation for
I've posted all three as GitHub suggestion comments on PR #3097. |
Validate
cog predict,cog run, andcog traininputs against the model's OpenAPI schema before building the image or starting the container. Bad inputs fail immediately instead of after a full build or container start.cog.yamlin Go — no Docker or Python needed.openapi_schemalabel after pull, before start.expected integer, got string.nullfor typed fields, matching runtime semantics.-istring values to schema types before validating.pkg/schema/openapi.GenerateSchema, one config-aware entry point shared by build and CLI preflight.pkg/schema/openapi.go→openapi_spec.go.Notes: