weiqingy commented on code in PR #1129:
URL: https://github.com/apache/flink-agents/pull/1129#discussion_r4055469668
##########
python/flink_agents/integrations/chat_models/tongyi_chat_model.py:
##########
@@ -247,34 +296,29 @@ def chat(
# popped on the line above, so a kwargs lookup would yield None on
every call
# and report every model incapable.
#
- # TODO(#912): the requested strategy is not visible here, so this check
- # cannot tell an explicit NATIVE request apart from one that merely
- # resolved to native. A caller asking for NATIVE on a model this
predicate
- # rejects therefore gets an unconstrained response instead of an error.
- # Once strategy resolution is wired up, NATIVE must either bypass this
- # capability check or fail explicitly.
- if output_schema is not None and
self.supports_native_structured_output(
- model_name
- ):
- # Resolved before the conflict test, so a payload with no native
- # translation does not raise over a response_format this branch was
- # never going to write. Tested before the schema is rendered,
because a
- # caller who supplies both a schema and a response_format has a
conflict
- # to resolve whatever the schema turns out to render to, and
reporting a
- # render failure instead would describe the wrong problem. The
name is
- # read off the model class, so this needs no rendered document.
+ # The feasibility half is asked rather than restated, so a caller
asking the
+ # same question gets the answer this branch acts on. A payload with no
native
+ # translation is reported infeasible there, so it never reaches the
conflict
+ # test below and cannot raise over a response_format this branch was
never
+ # going to write.
+ if self.can_apply_native_structured_output(
Review Comment:
Thanks for the review, and for running both suites. All six are addressed in
`dc8bfb03`, along with a description fix.
One correction worth surfacing: on the non-string model it's three
connections, not four. vLLM raises too. It inherits the query from OpenAI, so
it didn't show up in the first sweep.
The one I left alone is the `raw_kwargs` copy. It keeps all six connections
reading the same way, though it's a no-op on that path today. Does that trade
seem worth it to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
