feat: first pass at mot as a separate class#1348
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
AngeloDanducci
requested review from
a team,
jakelorocco and
nrfulton
as code owners
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
Overall this LGTM, Claude found one place you missed though. I also confirmed this won't cause issues outside a rough merge conflict with the work on #1315 and my work on #1216 (which I am wrapping up work on locally and had it compare against)
Missed a downstream caller: mellea/stdlib/components/intrinsic/_util.py:40.
_resolve_question dispatches on isinstance(model_input, CBlock) and otherwise raises ValueError(...not a supported type). Now that ModelOutputThunk is no longer a CBlock (and never a Component), a MOT model_input falls through to the raise where it previously returned model_input.value.
Reachable: last_turn() (base.py:1240) sets model_input = history[-2], which is a MOT whenever the two most recent context entries are both outputs — exercised by the public RAG functions rewrite_question, clarify_query, and check_answerability when called with question=None.
Fix is one line, since both types expose .value:
elif isinstance(model_input, (CBlock, ModelOutputThunk)):Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
This LGTM, and as long as it's the first in it won't have to worry about rebasing on the other MOT PRs
jakelorocco
left a comment
jakelorocco
left a comment
There was a problem hiding this comment.
Thoughts:
As an alternative, do you think it would be worth it to define an underlying structure that both CBlock and mots inherit from? This might just be recreating the issue we are trying to solve though.
Main thought for this PR as it is:
I think anywhere you see CBlock | Component (as well as match variants, etc...); ModelOutputThunk should basically be added. This would preserve existing behavior. Then we can work backwards and disallow the specific instances it doesn't make sense.
Other things:
- def _make_merged_kv_cache -> we should add the mot case / handling to this as well. Should be identical to cblock here.
- You should probably be able to generate from a model output thunk as well. This is slightly weird, but doesn't seem unheard of.
- While unlikely,
self._actionfor model output thunks should probably be allowed to correspond to a model output thunk as well (since they are accepted by generate?)
| self._grounding_context: dict[str, CBlock | ModelOutputThunk | Component] = { | ||
| k: blockify(v) if isinstance(v, str) else v |
There was a problem hiding this comment.
I believe blockify should be modified to accept mots and return them as is (same behavior as cblocks / components). Might be worth creating a new test case for this scenario as well.
| self._repair_string: str | None = None | ||
|
|
||
| def parts(self) -> list[Component | CBlock]: | ||
| def parts(self) -> list[Component | CBlock | ModelOutputThunk]: # type: ignore[override] # wider than Protocol; grounding_context can hold MOTs |
There was a problem hiding this comment.
I think the protocol itself should be expanded to support ModelOutputThunks so that this ignore override isn't needed. mots can technically represent arbitrary components (when using parsed_repr) and cblocks.
This may cause further issues in mify / mobjects as well (which are loosely coupled to the component protocol). They should just implement the behavior the same way (add mots to the typing).
There was a problem hiding this comment.
Makes sense, I'll take a stab at this.
I agree that it may just be recreating a similar issue but happy to work on that in a follow up if we think it's worth pursing 🤔 |
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
|
FYI I've set #1350 to merge when ready, so once that merges to main you'll need to rebase or merge with main and make sure nothing breaks since they touch |
|
#1350 has merged, once you rebase I'll take a final pass and approve |
3 participants
Pull Request
Issue
Fixes #269
Description
Moves ModelOutputThunk to its own class
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.