You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "jlon (via GitHub)" <gi...@apache.org> on 2026/06/26 06:55:16 UTC
[PR] [api][plan][python] Use request-scoped metric groups for chat token metrics [flink-agents]
jlon opened a new pull request, #861:
URL: https://github.com/apache/flink-agents/pull/861
## What
- Capture the current action metric group when a chat request is initiated.
- Record delayed chat token metrics against that captured group instead of the mutable metric group on cached chat model resources.
- Add Java and Python regression coverage for resource metric-group rebinding.
## Why
Cached resources can be rebound when another action retrieves the same resource before the original request records token usage. The delayed token metrics should stay under the action that initiated the request.
Closes #859.
## Tests
- mvn -pl api -DskipITs -Dspotless.check.skip=true -Dcheckstyle.skip=true -Dtest=BaseChatModelSetupTokenMetricsTest test
- mvn -pl plan -am -DskipITs -Dspotless.check.skip=true -Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false -Dtest=ChatModelActionTest,ChatModelActionRetryTest test
- bash -lc 'export PATH="$HOME/.local/bin:$PATH"; cd python && uv run --no-sync pytest flink_agents/api/chat_models/tests/test_token_metrics.py -q'
- bash -lc 'export PATH="$HOME/.local/bin:$PATH"; cd python && uv run --no-sync ruff format --check flink_agents/api/chat_models/chat_model.py flink_agents/api/chat_models/tests/test_token_metrics.py flink_agents/plan/actions/chat_model_action.py && uv run --no-sync ruff check flink_agents/api/chat_models/chat_model.py flink_agents/api/chat_models/tests/test_token_metrics.py flink_agents/plan/actions/chat_model_action.py'
- bash -lc 'export JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64; export PATH="$JAVA_HOME/bin:$PATH"; mvn -pl api,plan -am spotless:check -Dspotless.skip=false -DskipTests -DskipITs -Dcheckstyle.skip=true'
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [api][plan][python] Use request-scoped metric groups for chat token metrics [flink-agents]
Posted by "weiqingy (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [api][plan][python] Use request-scoped metric groups for chat token metrics [flink-agents]
Posted by "jlon (via GitHub)" <gi...@apache.org>.
jlon commented on PR #861:
URL: https://github.com/apache/flink-agents/pull/861#issuecomment-4911868955
Pushed ecd96c5 to address the review comments:
- Removed the 3-arg Java `recordTokenMetrics(...)` overload so token metrics are recorded only with an explicit request/action metric group.
- Made the Java metric group parameter non-null and added an action-layer skip when no metric group is available.
- Removed the Python fallback from `metric_group=None` to `self.metric_group`; Python now skips instead of recording under a possibly rebound resource group.
I kept the broader `Resource.metricGroup` removal out of this PR because that crosses resource binding and cross-language bridge behavior. This patch keeps the issue #859 fix focused on chat token metrics while avoiding the fallback contract that could recreate the bug.
Local verification:
- `mvn -pl api -DskipITs -Dspotless.check.skip=true -Dcheckstyle.skip=true -Dtest=BaseChatModelSetupTokenMetricsTest test`
- `mvn -pl plan -am -DskipITs -Dspotless.check.skip=true -Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false -Dtest=ChatModelActionTest,ChatModelActionRetryTest test`
- `cd python && uv run --no-sync pytest flink_agents/api/chat_models/tests/test_token_metrics.py -q`
- `cd python && uv run --no-sync ruff format --check ... && uv run --no-sync ruff check ...`
- `mvn -pl api,plan -am spotless:check -Dspotless.skip=false -DskipTests -DskipITs -Dcheckstyle.skip=true`
- `git diff --check`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [api][plan][python] Use request-scoped metric groups for chat token metrics [flink-agents]
Posted by "wenjin272 (via GitHub)" <gi...@apache.org>.
wenjin272 commented on code in PR #861:
URL: https://github.com/apache/flink-agents/pull/861#discussion_r3557289583
##########
python/flink_agents/api/chat_models/chat_model.py:
##########
@@ -273,8 +278,10 @@ 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 None, token
+ metrics are skipped.
"""
- metric_group = self.metric_group
if metric_group is None:
Review Comment:
One parity question: Python `_record_token_metrics(..., metric_group=None)` is a no-op, while Java `recordTokenMetrics(null, ...)` fails via `checkNotNull`. Should we align the lower-level helper contract?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [api][plan][python] Use request-scoped metric groups for chat token metrics [flink-agents]
Posted by "jlon (via GitHub)" <gi...@apache.org>.
jlon commented on code in PR #861:
URL: https://github.com/apache/flink-agents/pull/861#discussion_r3541595356
##########
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:
Updated in ecd96c5. `_record_token_metrics` now requires the metric group argument and no longer falls back to the resource-bound `self.metric_group`. The action path also skips token metric recording when no request/action metric group is available.
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelSetup.java:
##########
@@ -116,7 +116,22 @@ public void open() throws Exception {
* @param completionTokens the number of completion tokens
*/
public void recordTokenMetrics(String modelName, long promptTokens, long completionTokens) {
Review Comment:
Updated in ecd96c5. Removed the 3-arg Java `recordTokenMetrics(...)` overload and updated the tests to pass an explicit request/action metric group.
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelSetup.java:
##########
@@ -116,7 +116,22 @@ public void open() throws Exception {
* @param completionTokens the number of completion tokens
*/
public void recordTokenMetrics(String modelName, long promptTokens, long completionTokens) {
- FlinkAgentsMetricGroup metricGroup = getMetricGroup();
+ recordTokenMetrics(getMetricGroup(), modelName, promptTokens, completionTokens);
+ }
+
+ /**
+ * Record token usage metrics for the given model on the provided metric group.
+ *
+ * @param metricGroup the metric group captured when the request was initiated
+ * @param modelName the name of the model used
+ * @param promptTokens the number of prompt tokens
+ * @param completionTokens the number of completion tokens
+ */
+ public void recordTokenMetrics(
+ @Nullable FlinkAgentsMetricGroup metricGroup,
Review Comment:
Updated in ecd96c5. The Java `recordTokenMetrics(...)` metric group parameter is no longer nullable, and `ChatModelAction` now skips token metric recording before calling it when the action metric group is absent.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [api][plan][python] Use request-scoped metric groups for chat token metrics [flink-agents]
Posted by "joeyutong (via GitHub)" <gi...@apache.org>.
joeyutong commented on PR #861:
URL: https://github.com/apache/flink-agents/pull/861#issuecomment-4902263385
> Thanks for taking this on @jlon.
>
> Since `Resource` instances are shared across multiple Actions, reporting metrics using the `metricGroup` bound at creation time can lead to incorrect metric matching. This PR addresses the issue by passing the current Action’s `metricGroup` at reporting time, ensuring metrics are reported to the correct `Action` scope.
>
> In this case, I think `Resource` no longer need to hold a `metricGroup` field.
I agree. This PR fixes the concrete bug locally for chat token metrics by passing the request/action metric group at reporting time, instead of reading the mutable metric group currently bound to the cached resource. But the underlying model issue is broader than this one recording path.
A cleaner abstraction would be to separate the stable resource instance from the per-action resource-use context:
- `Resource` is a stable cached capability.
- `ResourceUseContext` carries action-scoped state, including the current action metric group.
- Metric reporting uses the current `ResourceUseContext`, not mutable state stored on the resource.
- Cross-language bridges propagate this context when invoking resource methods or resolving nested resources.
One possible shape is for `ctx.getResource(...)` to return a bound resource handle internally, i.e. the cached `Resource` plus the current `ResourceUseContext`, even if the public API is migrated gradually.
Under that model, `Resource` would no longer need a mutable `metricGroup` field. It would also simplify the cross-language propagation work in [#860](https://github.com/apache/flink-agents/pull/860): instead of making every Python wrapper participate in `setMetricGroup(...)`, the bridge can pass the current use context through the existing `callMethod` / `getResource` path. CC @weiqingy
This is probably a larger refactor than this PR should take on, but I agree with the direction: action-scoped metric state should not be owned by the cached `Resource` instance.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [api][plan][python] Use request-scoped metric groups for chat token metrics [flink-agents]
Posted by "wenjin272 (via GitHub)" <gi...@apache.org>.
wenjin272 commented on code in PR #861:
URL: https://github.com/apache/flink-agents/pull/861#discussion_r3527462853
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelSetup.java:
##########
@@ -116,7 +116,22 @@ public void open() throws Exception {
* @param completionTokens the number of completion tokens
*/
public void recordTokenMetrics(String modelName, long promptTokens, long completionTokens) {
- FlinkAgentsMetricGroup metricGroup = getMetricGroup();
+ recordTokenMetrics(getMetricGroup(), modelName, promptTokens, completionTokens);
+ }
+
+ /**
+ * Record token usage metrics for the given model on the provided metric group.
+ *
+ * @param metricGroup the metric group captured when the request was initiated
+ * @param modelName the name of the model used
+ * @param promptTokens the number of prompt tokens
+ * @param completionTokens the number of completion tokens
+ */
+ public void recordTokenMetrics(
+ @Nullable FlinkAgentsMetricGroup metricGroup,
Review Comment:
In practice, we now require that the `metricGroup` of the current action be passed in when reporting metrics. Therefore, I believe `metricGroup` should not be nullable.
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/BaseChatModelSetup.java:
##########
@@ -116,7 +116,22 @@ public void open() throws Exception {
* @param completionTokens the number of completion tokens
*/
public void recordTokenMetrics(String modelName, long promptTokens, long completionTokens) {
Review Comment:
It appears that only tests call this method, so I think it can be removed directly.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [api][plan][python] Use request-scoped metric groups for chat token metrics [flink-agents]
Posted by "wenjin272 (via GitHub)" <gi...@apache.org>.
wenjin272 merged PR #861:
URL: https://github.com/apache/flink-agents/pull/861
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org