weiqingy commented on code in PR #783:
URL: https://github.com/apache/flink-agents/pull/783#discussion_r3366822560
##########
python/flink_agents/api/chat_models/chat_model.py:
##########
@@ -226,6 +226,6 @@ def _record_token_metrics(
if metric_group is None:
return
- model_group = metric_group.get_sub_group(model_name)
+ model_group = metric_group.get_sub_group("model", model_name)
Review Comment:
This same migration — `get_sub_group(model_name)` → `get_sub_group("model",
model_name)` — also lives in `vector_store_long_term_memory.py`'s
`_report_token_metrics` (around line 267), where long-term-memory token usage
is still recorded under the flat `get_sub_group(metric["model_name"])`. On
`main` that path was the mem0 file #760 migrated, but mem0 doesn't exist on
`release-0.2`, so the vector-store LTM is where it lives here — and it sits
outside this PR's diff. Left as-is, chat-model token metrics pick up the
`model.` prefix while LTM metrics keep `long-term-memory.<model>.promptTokens`,
and since the rename is breaking it'd take a second breaking change to bring
LTM in line later. Was leaving it out a deliberate scope call, or would it be
worth folding the same one-line change in here?
(`test_vector_store_long_term_memory.py` mocks `get_sub_group` with an
arg-agnostic autospec, so it shouldn't need a test rewrite.)
--
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]
