implement union type support for streaming actions#813
Conversation
|
Thanks for picking this up and for writing out the investigation so clearly. One small thought from my earlier pass on this issue: adding object to the union does make the type checker accept union values, but it also effectively widens stream_type to almost any value. That may still be the pragmatic choice here, but maintainers may prefer either a more explicit alias/comment around why the parameter is intentionally broad, or a small runtime/type-helper path that recognizes union forms while keeping the public intent narrow. The focused tests for both yping.Union[...] and ModelA | ModelB look useful. Thanks again for moving this forward. |
|
Thank you for your suggestions. That's a good catch. I addressed the issues that you mentioned above. |
|
Thank you for following up and tightening that path. I appreciate you taking the suggestion constructively, and the updated direction looks much clearer to me. Nice work. |
| if stream_type is None: | ||
| # TODO -- derive from the signature | ||
| raise ValueError(f"stream_type is required for function: {fn.__qualname__}") | ||
| if not _is_valid_stream_type(stream_type): |
There was a problem hiding this comment.
This looks like the key behavior change in the PR, so I think we should add negative-path coverage for it as well. Right now the new tests show that valid unions are accepted, but they don’t pin the rejection behavior for invalid cases like stream_type=int or stream_type=Union[TextChunk, int]. Could we add a couple of pytest.raises(ValueError) assertions for those?
There was a problem hiding this comment.
Yes. That's valid. I added more tests for existing positive path and negative path.
| state_input_type: Type["BaseModel"], | ||
| state_output_type: Type["BaseModel"], | ||
| stream_type: Union[Type["BaseModel"], Type[dict]], | ||
| stream_type: Union[Type["BaseModel"], Type[dict], object], |
There was a problem hiding this comment.
This fixes the usability issue, but it also broadens the public type hint a lot. Is that tradeoff intentional? We’re now depending on runtime validation for the real constraint, so static tooling no longer gets much help from the signature.
There was a problem hiding this comment.
Yes. A typing.Union[...] value has no precise static type that mypy evaluates the expression as object, so any narrower hint would produce false positives for the exact use case this PR enables and we support 3.9, where pipe syntax isn't available. This is the same pattern FastAPI uses for response_model: Any. The real constraint is enforced at decoration time by _is_valid_stream_type, and the rejection behavior is now pinned by negative-path tests (per your other comment). I've named the type of alias and documented the tradeoff at the definition site, so it reads as deliberate.
| # ============================================================================ | ||
|
|
||
|
|
||
| def test_streaming_action_typing_union_two_models(): |
There was a problem hiding this comment.
The happy-path coverage here is good, but I think we’re still missing one or two failure-mode tests for the new validator. Adding invalid stream_type cases would make the review much easier because it would pin the exact boundary this PR is introducing.
There was a problem hiding this comment.
Thank you for your review and feedback. I addressed this.
Summary
Extends the
@streaming_action.pydantic()decorator to accept union types likeModel1 | Model2orUnion[Model1, Model2]) for thestream_typeparameter. Previously, only single BaseModel types ordictwere accepted, limiting flexibility for streaming actions that yield multiple result types.Changes
How I tested this
Unit Tests: Created comprehensive test suite with 10 tests that all pass
Notes
Resolved: #607
Checklist