weiqingy commented on code in PR #861:
URL: https://github.com/apache/flink-agents/pull/861#discussion_r3502154704
##########
python/flink_agents/api/chat_models/chat_model.py:
##########
@@ -273,8 +278,12 @@ def _record_token_metrics(
The number of prompt tokens
completion_tokens : int
The number of completion tokens
+ metric_group : MetricGroup | None
+ The metric group captured when the request was initiated. If not
provided,
+ this resource's currently bound metric group is used.
"""
- metric_group = self.metric_group
+ if metric_group is None:
Review Comment:
The two languages now document slightly different contracts for the
absent-group case: Java's 4-arg `recordTokenMetrics` treats a `null` group as
"skip recording" (`BaseChatModelSetup.java:135`), while here `metric_group is
None` falls back to `self.metric_group` — the live, possibly-rebound group.
Every real call site passes a captured group (`chat_model_action.py:334`), so
there's no impact on today's flow. The part I'm less sure about: a future
caller passing `None` expecting Java parity would record under whatever action
is currently bound — the #859 scenario again on the Python side, where Java
would no-op instead. Is the asymmetry intentional? If the `= None` default is
only there to keep the param optional for the test helper, dropping it so
callers must pass the captured group would match Java's no-fallback contract;
alternatively a one-line javadoc note that Java's `null` means skip would at
least make each side self-documenting.
--
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]
