fix(sampling): migrate budget forcing to mot.generation.usage; drop _meta["usage"] writes#1288
Conversation
…meta["usage"] writes Migrate the only in-tree consumer (`think_budget_forcing`) off `mot._meta["usage"]` and onto `mot.generation.usage`, then drop the now-redundant `_meta["usage"]` writes from all five backends' `generate_from_raw` paths. The additive half (writing `mot.generation.usage` from raw paths) landed with generative-computing#1264. Budget forcing now raises a named `RuntimeError` when a sub-call returns `mot.generation.usage = None` instead of letting a confusing `TypeError` bubble out of arithmetic on `None`. Also replaces an implicit `IndexError` in the `think_max_tokens=0`/`answer_suffix=None` no-sub-calls edge case with a named error. Closes generative-computing#1218 Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
|
Comment by Claude: Optional but Recommended: Add Error Case Tests The PR introduces two new RuntimeError paths that should ideally have test coverage:
These are edge cases that might benefit from unit tests in test/stdlib/sampling/test_think_budget_forcing.py, but they're currently not critical since the budget forcing tests are integration/e2e level (marked with @pytest.mark.e2e, @pytest.mark.qualitative). Recommendation The PR can merge without test changes, but to be thorough, you could add a unit test like: This would ensure the new error paths are exercised, but it's not blocking — existing tests will validate the happy path works correctly. |
This did come up when writing but I decided not to since there are no unit tests currently, so I would be introducing them. If including unit tests feels like a blocker I can add them. |
9b3a3c2
Pull Request
Issue
Fixes #1218
Description
think_budget_forcingto readmot.generation.usageexclusively. Raises a namedRuntimeErrorwhen usage isNone, replacing the previous opaqueTypeErrorfrom arithmetic onNone(Ollama'seval_countcan beNone)._meta["usage"]writes from all five backends'generate_from_rawpaths now thatmot.generation.usageis the unified field. The additive half (writing the field from raw paths) landed with refactor(backends): move generate_from_raw hook firing into Backend base class #1264.IndexErrorin the no-sub-calls edge case (think_max_tokens=0+answer_suffix=None) with a namedRuntimeError.Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.