[OMEGA-350] Add unit tests for LLM token-budget handling - #350
Conversation
I disagree. This could lead to hidden additional costs. I believe that sending a message to the chat is enough in this case. We should not make additional hidden requests here. I also discovered several critical issues in these changes:
And what happens if a normal reply never comes back? The agent will still look like it's completely ignoring the user.
Why? The user paid for this response, but we just discard it. I believe a better approach here is to simply append "The agent's answer was cut off..." to the LLM response. In addition, as I see it, the refactoring was mostly caused by the retry implementation. If we decide that it's not necessary, then most of the refactoring is not necessary either, so we could leave only:
These changes could be added directly to #337 after I apply the changes described below, and this PR could then be closed. What I should update in #337 in that case:
cc: @vsbogd, please take a look at both PRs and share your opinion when you have some free time. |
We discussed this before and agreed that automatic retry is not needed. My suggestion was not do this because model without reasoning has poor performance. But at the moment I think I didn't take into account different kinds of LLM requests we have. We have reactive agent iterations when agent responses to some request from the user. In such situation notifying user that reasoning is failed because of budget it correct. We have proactive agent iterations when agent pursues its own goals or trying to help user with previous tasks. For this mode it may look surprisingly to user when agent reports about budget exhausted without any input from user. In the last case it sounds more logical to notify LLM about results of the previous iteration. And retrying request automatically without reasoning makes more sense.
|
054dda0 to
783ef67
Compare
|
@paul-v-snet @vsbogd the retry is dropped and the branch is rebased onto On the retry, agreed, and two of Paul's points were right: GLM-5.3 and newer cannot disable reasoning, and I did not check for
That is what the once-per-streak change is for. On
In V1 the loop parses the reply as text, so a truncated command runs before anyone reads the note. In the 120 run a cut-off Tested on the rebased branch: CI 65 / 149 / 5+1, plus live runs on OpenRouter with Neither change depends on the retry, so they can land here or go straight into #337, whichever you prefer. |
783ef67 to
49375ed
Compare
|
@paul-v-snet @vsbogd trimmed this PR to the tests. The retry, the once-per-streak notice and the truncated-reply guard are all out. On the notice in history you are right, and the screenshot settles it: the agent reads its own error and splits the answer by itself, and my flag would have taken that away. My numbers for the flooded prompt came from The truncated reply is still worth fixing, in one run a cut-off What is left in this PR covers your own changes: the |
Description
Follow-up to #337, based on its branch. Unit tests only, no behaviour change.
Autotests/unit/test_llm_budget.py(15 tests), registered inrun_mandatory, covers what #337 added: thefinish_reasonandincomplete_reasonchecks before the notice, the notice itself on an empty reply that ran out of budget for OpenRouter, OpenAI and ASI:One, the ASI:One reasoning budget mapping, the OpenRouter reasoning body,[LLM_USAGE]at INFO on both APIs, and an API error returning an empty string. The provider modules are loaded by file path withopenaiandconfigstubbed, so the tests need no container, network or token.Earlier versions of this PR also changed provider behaviour: a retry without reasoning, one notice per streak, and keeping a truncated reply out of the loop. All three are dropped after the discussion here and in #337.
How Has This Been Tested?
CI:
tests/pytest.sh65 passed, Phase 1 144 passed (129 + 15 new), Phase 2 5 passed and 1 skipped, MeTTa tests green.Checklist