weiqingy commented on PR #843: URL: https://github.com/apache/flink-agents/pull/843#issuecomment-4975215128
Thanks @wenjin272, I agree with both objections and the order you're suggesting. **`modelParams` as the schema channel.** It's the wrong container: `modelParams` is provider-facing generation params, the schema is framework execution metadata. Routing it through there costs a reserved key, a strip in `BaseChatModelSetup.chat()`, a pop in the native connection (each in both languages), and three test files that only prove the key never reaches an SDK. All it buys is an unchanged `chat()` signature, which is thin at 0.3-SNAPSHOT. I'll also retract a claim from my own design doc: that an explicit parameter would break every connection. It won't. In Java a defaulted overload `chat(messages, tools, modelParams, outputSchema)` needs no edits to non-native connections. In Python it's one named `output_schema=None` per connection, and that parameter is exactly what kills the leak, since today the schema rides inside the `**kwargs` forwarded to the SDK. **`hasTools`.** The bigger issue is that it makes the feature inert: `BaseChatModelSetup` binds tools once in `open()` and passes them on every call, so the native path never fires for a `ReActAgent` that has both tools and an `output_schema`, which is the case it's for. `hasTools` is a proxy for "is this the final turn," and only the caller knows that. **The root cause is the missing finalization step.** The gate only exists because there's no post-loop call. Make final-output structuring a separate call (full history, schema bound, no tools) and both problems disappear: the caller declares when structure is wanted, and the schema is a real argument. That ordering removes machinery instead of adding it. Proposed shape: 1. Explicit schema on the chat path (deletes the reserved key, strip, pop, and no-leak tests). 2. Finalization as a dedicated post-loop call at `ChatModelAction`'s no-tool-calls branch. One caveat: the LangChain doc you linked has moved past the post-loop-call shape and now uses `ProviderStrategy` (native) plus `ToolStrategy` (a synthetic tool called inside the loop, no extra call). So the extra round-trip is a cost that prior art avoids, which is my first question below. 3. Split policy from capability: the setup declares policy (`auto`/`native`/`prompt`), the connection checks capability against the effective model at request-build time. Under `auto`, an incapable model falls back to prompt instead of failing. On `with_structured_output` itself: it returns a per-call object, but our `ChatModelSetup` is a named `Resource`, `open()`-ed once and re-resolved by name per record. A derived Setup has no registry name and can't cross the Pemja bridge; mutating the shared one is a data race in a streaming operator. The part that ports is the declarative strategy selection, which is where (3) comes from. Open to a different shape if you had one in mind. A few calls where I have a lean but want your read: 1. The post-loop call costs one extra LLM round-trip per record. I'd start with that, since it's simpler and the parse side already assumes a no-tool-calls final turn, and treat the forced-tool alternative (`ToolStrategy`) as a later optimization. The mechanism exists on-repo (`AnthropicChatModelConnection` strict tool use, `:186-200`) but doesn't force the call, so it'd be new behavior. 2. For how `auto` decides capability, I'd avoid a known-not-supported list (it rots on every provider release) and lean toward native only when the user explicitly configures a capable model, prompt otherwise. 3. I'd keep finalization as its own follow-up PR (a #280 sub-issue), as we'd agreed, leaving this PR the foundation. Let me know if folding it in would make the review easier. WDYT? -- 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]
