fix: support list[X] | None inputs + integration tests for PEP 604 union File/Path coercion#2882
fix: support list[X] | None inputs + integration tests for PEP 604 union File/Path coercion#2882markphelps wants to merge 10 commits intomainfrom
Conversation
Add five txtar integration tests verifying that coglet correctly handles PEP 604 pipe-union syntax (X | None) for File and Path input coercion: - pep604_file_or_none_input: File | None via predict + serve - pep604_path_or_none_input: Path | None via predict + serve - pep604_list_file_or_none_input: list[File] | None via predict + serve - pep604_list_path_or_none_input: list[Path] | None via predict + serve - pep604_string_url_not_coerced: str | None with URL is NOT coerced (negative)
1bdb23c to
236c9aa
Compare
|
Hi @markphelps, great follow-up on #2878 — the new PEP 604 tests are clear and focused. One suggestion: for File/Path union cases, the HTTP section only validates the omitted-input ( I can draft a representative test case to reuse across the 4 fixtures if you’d like. |
…ema gen The Python SDK and Go static schema generator only supported three Repetition variants (Required, Optional, Repeated), so type annotations like `list[File] | None` and `list[Path] | None` would fail: - Python SDK raised "Optional cannot have nested type list" - Go schema gen silently discarded the list semantics, producing an optional scalar schema instead of a nullable array Add OptionalRepeated to both type systems, threading it through type resolution, normalization, JSON schema generation, and input validation.
cog.File objects are BytesIO at runtime (not 'File'), and cog.Path reports as 'Path' (not 'PosixPath'). Check count and content instead of type names, matching the pattern used by the single-element tests.
txtar file sections include trailing newlines, which leak into the joined output string causing a line break that the stdout matcher can't match across. Strip content before joining.
@erayack that would be very helpful thank you! |
|
Can you grant me access, @markphelps? Or you could cherry-pick. erayack@d061c36 |
|
@markphelps Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
I'm Bonk, and I've done a quick review of your PR. This PR adds Issues found (ranked by severity):
These are suggestions for improvement rather than blockers. The implementation correctly handles the edge cases, has excellent backward compatibility, and the integration tests provide good coverage. |
@erayack can you open a PR on this PR or does it have to wait until this is on main? |
@markphelps, You can take it now; it doesn’t need to wait for main. |
|
Looking at this PR, I can see it's a well-structured fix for supporting The changes look correct:
The implementation correctly fixes the bug where the type system couldn't represent "optional AND repeated", which was causing issues in both the Python SDK and Go static schema generator. LGTM |
|
Based on my thorough review of PR #2882, I can confirm this is a well-implemented fix for supporting Summary of changes:
The implementation correctly fixes the bug where the type system couldn't represent "optional AND repeated", which was causing the Python SDK to raise All changes maintain backward compatibility, follow existing code patterns, and include appropriate error handling. The integration tests verify both CLI ( LGTM |
|
Looking at this PR, I can see it's a well-structured fix for supporting The changes look correct:
The implementation correctly fixes the bug where the type system couldn't represent "optional AND repeated", which was causing issues in both the Python SDK and Go static schema generator. LGTM |
Summary
Adds support for
list[X] | None(optional repeated) type annotations in predictor inputs, and integration tests for the PEP 604 union support introduced in #2878.The
list[File] | Noneandlist[Path] | Noneintegration tests uncovered a bug in both the Python SDK and Go static schema generator: theRepetitiontype system only had three variants (Required,Optional,Repeated) and couldn't represent "optional AND repeated". This caused:ValueError: Optional cannot have nested type listduringcog buildschema validationChanges
Fix:
OptionalRepeatedsupportAdded a fourth
Repetitionvariant (OptionalRepeated/OPTIONAL_REPEATED) to both the Python SDK and Go schema generator, threaded through:python/cog/_adt.py—FieldType.from_type()detectslist[X] | Nonepython/cog/_adt.py—normalize(),json_encode(),json_decode()python/cog/_inspector.py— default handling, constraint checkspython/cog/_schemas.py— nullable flagpkg/schema/types.go—ResolveFieldType()for bothOptional[list[X]]andlist[X] | Nonepkg/schema/openapi.go— nullable flag, array typeIntegration tests
pep604_file_or_none_inputFile | None— file input is coerced, None default workspep604_path_or_none_inputPath | None— path input is coerced, None default workspep604_list_file_or_none_inputlist[File] | None— multiple file inputs coerced, None default workspep604_list_path_or_none_inputlist[Path] | None— multiple path inputs coerced, None default workspep604_string_url_not_coercedstr | Nonewith URL — negative test ensuring the PEP 604 fix doesn't regress #2868Each test exercises both the CLI (
cog predict) and serve (cog serve+curl) code paths.Unit tests
TestOptionalListPipeNone— Go parser:list[Path] | NoneTestOptionalListTypingOptional— Go parser:Optional[List[str]]TestOptionalListFileInput— Go parser:list[File] | NoneTestInputOptionalRepeatedType— Go OpenAPI: nullable array schema, not requiredMerge strategy
Merge #2878 first, then rebase this PR onto main.