Skip to content

reject file path input defaults #3094

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.

Sign up for GitHub

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

Open
sarptandoven wants to merge 1 commit into
base: main
Choose a base branch
from
+115 −11
Open

reject file path input defaults #3094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions pkg/schema/python/inputs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,16 @@ import (
)

type inputCallInfo struct {
Default *schema.DefaultValue
Description *string
GE *float64
LE *float64
MinLength *uint64
MaxLength *uint64
Regex *string
Choices []schema.DefaultValue
Deprecated *bool
Default *schema.DefaultValue
DefaultUnresolved bool
Description *string
GE *float64
LE *float64
MinLength *uint64
MaxLength *uint64
Regex *string
Choices []schema.DefaultValue
Deprecated *bool
}

type inputMethodInfo struct {
Expand Down Expand Up @@ -216,6 +217,11 @@ func resolveInputReference(node *sitter.Node, source []byte, registry *inputRegi
case "default":
if val, ok := parseDefaultValue(callNode, source); ok {
resolved.Default = &val
resolved.DefaultUnresolved = false
} else {
none := schema.DefaultValue{Kind: schema.DefaultNone}
resolved.Default = &none

@markphelps markphelps Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch overall. I think this branch needs one small fix here before it lands: this else changes behavior outside the Path/File case.

resolved starts as a copy of methodInfo.BaseInfo, so it can already have a default from the helper method. Before this PR, if a call-site default was not a literal, we left that base default alone. Now we replace it with None.

I verified this with a non-file input like:

MY = "base.png"

class Helper:
    @staticmethod
    def img(default: str) -> Input:
        return Input(description="Image", default="base.png")

class Predictor(BasePredictor):
    def predict(self, image: str = Helper.img(default=MY)) -> str:
        return "ok"

On this branch the parsed default becomes None; before the PR it stayed "base.png". That is a silent schema change for non-file inputs.

Could we keep the new unresolved marker without clobbering the base default?

} else {
    // Preserve the base default for non-file types; file/path types are still
    // rejected by validateInputFieldWithInfo via DefaultUnresolved.
    resolved.DefaultUnresolved = true
}

I checked this locally: it preserves the old non-file behavior, and file/path defaults through this path still get rejected because validateInputFieldWithInfo sees DefaultUnresolved.

resolved.DefaultUnresolved = true
}
case "description":
if s, ok := parseStringLiteral(callNode, source); ok {
Expand Down Expand Up @@ -285,6 +291,14 @@ func inputFieldWithInfo(name string, order int, inputType schema.InputType, fiel
return field
}

func validateInputFieldWithInfo(field schema.InputField, info inputCallInfo) error {
if info.DefaultUnresolved && field.InputType != nil && schema.InputTypeContainsFileOrPath(*field.InputType) {
return schema.WrapError(schema.ErrDefaultNotResolvable,
fmt.Sprintf("parameter '%s': Path and File defaults cannot be statically resolved", field.Name), nil)
}
return schema.ValidateInputField(field)
}

func firstParamIsSelf(params *sitter.Node, source []byte) bool {
for _, child := range AllChildren(params) {
if child.Type() == "identifier" {
Expand Down Expand Up @@ -399,13 +413,13 @@ func parseTypedDefaultParameter(node *sitter.Node, source []byte, order int, ctx
return schema.InputField{}, err
}
field := inputFieldWithInfo(name, order, inputType, fieldType, info)
return field, schema.ValidateInputField(field)
return field, validateInputFieldWithInfo(field, info)
}

// 2. Reference to Input() via class attribute or static method
if info, ok := resolveInputReference(valNode, source, ctx.registry); ok {
field := inputFieldWithInfo(name, order, inputType, fieldType, info)
return field, schema.ValidateInputField(field)
return field, validateInputFieldWithInfo(field, info)
}

// 3. Plain default — must be statically resolvable
Expand Down Expand Up @@ -469,6 +483,7 @@ func parseInputCall(node *sitter.Node, source []byte, paramName string, scope mo
if !ok {
none := schema.DefaultValue{Kind: schema.DefaultNone}
val = none
info.DefaultUnresolved = true
}
info.Default = &val
case "default_factory":
Expand Down
60 changes: 60 additions & 0 deletions pkg/schema/python/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,66 @@ class Predictor(BasePredictor):
require.False(t, scale.IsRequired())
}

func TestPathAndFileInputsRejectDefaults(t *testing.T) {

@markphelps markphelps Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add one regression test for the static-method/reference path too? The new table is good for direct Input(...), but it would not have caught the default-clobbering bug above because that lives in resolveInputReference.

A useful test would be the non-file helper-method case from the other comment: helper returns Input(default="base.png"), caller passes default=MY, and we assert the parsed default is still "base.png".

It would also be worth adding one Path/File case through this path, e.g. image: Path = Helper.img(default=Path("x.png")), to prove the new DefaultUnresolved rejection still works for registry references.

tests := []struct {
name string
source string
wantErr schema.SchemaErrorKind
}{
{
name: "path string default",
source: `
from cog import BasePredictor, Input, Path

class Predictor(BasePredictor):
def predict(self, image: Path = Input(default="image.png")) -> str:
return "ok"
`,
wantErr: schema.ErrUnsupportedType,
},
{
name: "file string default",
source: `
from cog import BasePredictor, File, Input

class Predictor(BasePredictor):
def predict(self, image: File = Input(default="image.png")) -> str:
return "ok"
`,
wantErr: schema.ErrUnsupportedType,
},
{
name: "path constructor default",
source: `
from cog import BasePredictor, Input, Path

class Predictor(BasePredictor):
def predict(self, image: Path = Input(default=Path("image.png"))) -> str:
return "ok"
`,
wantErr: schema.ErrDefaultNotResolvable,
},
{
name: "path list default",
source: `
from cog import BasePredictor, Input, Path

class Predictor(BasePredictor):
def predict(self, images: list[Path] = Input(default=["image.png"])) -> str:
return "ok"
`,
wantErr: schema.ErrUnsupportedType,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
se := parseErr(t, tt.source, "Predictor", schema.ModePredict)
require.Equal(t, tt.wantErr, se.Kind)

@markphelps markphelps Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small optional tightening: these assertions would be a little stronger if the table also checked a message substring.

The Kind assertion is meaningful, but a wantMsg column would pin the two user-facing paths too:

  • literal Path/File default: defaults are not supported on Path or File inputs
  • unresolvable constructor default: Path and File defaults cannot be statically resolved

Not a blocker, just a cheap way to make the regression tests more explicit.

})
}
}

// ---------------------------------------------------------------------------
// Optional / union inputs
// ---------------------------------------------------------------------------
Expand Down
29 changes: 29 additions & 0 deletions pkg/schema/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,38 @@ func ValidateInputField(field InputField) error {
return errUnsupportedType("constraints and choices are not supported on union inputs")
}
}
if inputFieldContainsFileOrPath(field) && field.Default != nil && field.Default.Kind != DefaultNone {
return errUnsupportedType("defaults are not supported on Path or File inputs")
}
return nil
}

// InputTypeContainsFileOrPath reports whether a recursive input type contains
// a Path or File primitive. It is used by parser frontends that need to reject

@markphelps markphelps Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny doc nit: this helper is also used by ValidateInputField through inputFieldContainsFileOrPath, not just by parser frontends.

Maybe:

// It is used by ValidateInputField and parser frontends that need to reject
// unsupported file/path defaults after annotation resolution.

Optional, but it would make the comment match the actual call sites.

// unsupported file/path defaults after annotation resolution.
func InputTypeContainsFileOrPath(inputType InputType) bool {
switch inputType.Kind {
case InputKindPrimitive:
return inputType.Primitive == TypePath || inputType.Primitive == TypeFile
case InputKindArray:
return inputType.Elem != nil && InputTypeContainsFileOrPath(*inputType.Elem)
case InputKindUnion:
for _, variant := range inputType.Variants {
if InputTypeContainsFileOrPath(variant) {
return true
}
}
}
return false
}

func inputFieldContainsFileOrPath(field InputField) bool {
if field.InputType != nil {
return InputTypeContainsFileOrPath(*field.InputType)
}
return field.FieldType.Primitive == TypePath || field.FieldType.Primitive == TypeFile
}

// PredictorInfo is the top-level extraction result.
type PredictorInfo struct {
Inputs *OrderedMap[string, InputField]
Expand Down
Loading