fix: allow act to operate on CBlock and ModelOutputThunk#1429
fix: allow act to operate on CBlock and ModelOutputThunk#1429AngeloDanducci wants to merge 4 commits into
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
planetf1
left a comment
There was a problem hiding this comment.
Overall this is a solid fix for #356 — widening action to accept CBlock/ModelOutputThunk and guarding .parse() calls is the right approach, and the type-hierarchy work is careful. Three things worth resolving before merge, mainly around the strategy default change in act/aact.
| *, | ||
| requirements: list[Requirement] | None = None, | ||
| strategy: SamplingStrategy | None = RejectionSamplingStrategy(loop_budget=2), | ||
| strategy: SamplingStrategy | None = None, |
There was a problem hiding this comment.
This overload still defaults strategy to None, but the runtime assert a few lines down requires strategy is not None when return_sampling_results=True. So act(..., return_sampling_results=True) without an explicit strategy now type-checks but crashes at runtime — a pattern that worked before this PR, since the old default (RejectionSamplingStrategy(loop_budget=2)) satisfied the assert. Same issue on the aact Literal[True] overload in session.py.
Could these Literal[True] overloads require strategy explicitly instead of defaulting it to None? That would catch this at the type-checker instead of at runtime. Failing that, at least make the failure survive -O:
| strategy: SamplingStrategy | None = None, | |
| if return_sampling_results and strategy is None: | |
| raise ValueError( | |
| "return_sampling_results=True requires an explicit `strategy`." | |
| ) |
There was a problem hiding this comment.
Dropped the strategy=None default on all three return_sampling_results=Literal[True] overloads now required at thetype-checker. Converted the runtime assert -> raise ValueError.
| *, | ||
| requirements: list[Requirement] | None = None, | ||
| strategy: SamplingStrategy | None = RejectionSamplingStrategy(loop_budget=2), | ||
| strategy: SamplingStrategy | None = None, |
There was a problem hiding this comment.
With this default now None, aact(action, requirements=[...]) without an explicit strategy just logs a warning and skips validation entirely — no retry, no check. Before this PR the default strategy enforced requirements automatically. (instruct/ainstruct are unaffected — they keep their own default.)
Intentional? If so, worth a line in the strategy docstring saying so.
There was a problem hiding this comment.
I think we do want to disable defaulting to no strategy, but I agree with this. We should log on requirements / mention in the docstring / raise an assertion if requirements are provided without a sampling strategy.
There was a problem hiding this comment.
Kept this as a warning, will need a follow up issue. I added a raise but it broke an internal caller: generative stubs passes requirements to act() with strategy=None and renders them into the prompt.
test_genstub.py::test_requirement failed so I added a warning, and left a code comment pointing about a follow up.
| backend: the backend used to generate the response. | ||
| requirements: used as additional requirements when a sampling strategy is provided. | ||
| strategy: a SamplingStrategy that describes the strategy for validating and repairing/retrying for the instruct-validate-repair pattern. None means that no particular sampling strategy is used. | ||
| strategy: a SamplingStrategy that describes the strategy for validating and repairing/retrying for the instruct-validate-repair pattern. Defaults to None, meaning no sampling strategy is used. |
There was a problem hiding this comment.
This docstring update is accurate but doesn't quite capture the practical effect: with strategy=None and await_result still False by default, await aact(...) now returns an uncomputed thunk where it previously returned a computed one. Worth naming that here:
| strategy: a SamplingStrategy that describes the strategy for validating and repairing/retrying for the instruct-validate-repair pattern. Defaults to None, meaning no sampling strategy is used. | |
| strategy: a SamplingStrategy that describes the strategy for validating and repairing/retrying for the instruct-validate-repair pattern. Defaults to None — in that case the result is uncomputed unless `await_result=True` is also passed. |
There was a problem hiding this comment.
Expanded strategy docstrings (act/aact, both functional.py and session.py) to state no-validation + uncomputed-thunk behavior, added Raises: sections
| # CBlocks and ModelOutputThunks have no `.parse`, so fall back to the | ||
| # raw value (mirrors how backends handle non-Component actions). | ||
| result.parsed_repr = ( | ||
| action.parse(result) | ||
| if isinstance(action, Component) | ||
| else result.value # type: ignore[assignment] | ||
| ) |
There was a problem hiding this comment.
I think ModelOutputThunks might be a special case. A mot can actually be of type MOT[] meaning that this was output generated from a component that requested a special output type (ie the mot was previously parsed).
It's probably a very rare situation where you would want to use a mot for an action, but I think we should actually act as if the is the action here, meaning we would use it to parse.
I may be overthinking this / missing some issues this interpretation causes. If so, please flag those before we commit to this path.
There was a problem hiding this comment.
And if we go this route, can you please add a test to the test/typing folder that checks for this double parse situation?
There was a problem hiding this comment.
Not sure this is cleanly implementable currently without a larger MOT data model change.
From my understand it works as such:
When a MOT is computed it dispatches on the originating action:
- Component() -> parsed_repr = action._parse(self) (the typed Component result, S)
- CBlock() -> parsed_repr = self.value (raw string)
- ModelOutputThunk() -> parsed_repr = self.value (raw string)
Thee problems as a result:
- A MOT doesn't retain its originating Component — only the parsed value. To re-parse we'd need the Component object (self._call.action), but that isn't carried through copy/deepcopy; only parsed_repr (the already-parsed result instance) survives. parsed_repr is a value, not a re-invokable parser.
- Most MOTs aren't MOT[Component] at all. Any MOT generated from a CBlock or another MOT stores parsed_repr = value (a string). So there's no generic, reliable way to recover a re-parseable Component from an arbitrary MOT action — only the subset that originated from a Component would even have one.
- Uncomputed MOTs have parsed_repr = None, so the inner type isn't inspectable until after compute anyway.
Unless I'm misunderstanding and we don't need to reparse.
Given that, I've kept the raw-value fallback (which mirrors how backends handle non-Component actions) and expanded the code comment to explain why a MOT can't re-parse currently.
| sampled_results.append(result) | ||
| sampled_scores.append(constraint_scores) | ||
| sampled_actions.append(next_action) | ||
| sampled_actions.append(next_action) # type: ignore[arg-type] |
There was a problem hiding this comment.
Can you please go ahead and update these types that were previously Component only that now can be mots / cblocks? or open an issue for it? I think you had previously covered most of these cases in a different PR.
There was a problem hiding this comment.
Updated - had some knock on effects with typing - added SampleActionType = Component | CBlock | ModelOutputThunk as a result.
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
| # The kinds of action a sampling strategy may operate on. Originally `Component` | ||
| # only; widened to include `CBlock` and `ModelOutputThunk` so that `act`/`aact` | ||
| # can sample over non-Component actions (see #356). Only `Component` carries | ||
| # `.parse()` semantics; the others are carried through as opaque spans. | ||
| SampleActionType = Component | CBlock | ModelOutputThunk |
There was a problem hiding this comment.
Small nit: do you think we should just define a new type that handles all these for other places all three are allowed as well? It seems slightly weird to have this union type only for sampling.
There was a problem hiding this comment.
Yeah I think it makes sense to do this as it's already a bit pervasive. Seems out of scope for this - I'll open a followup issue.
Pull Request
Issue
Fixes #356
Description
mfuncs.act(CBlock("What is 1+1?"), ...) crashed with AttributeError: 'CBlock' object has no attribute 'parse'. act/aact defaulted to RejectionSamplingStrategy(loop_budget=2), so even a bare CBlock entered the sampling loop, which unconditionally called action.parse(result) — a method only Component has.
Per the maintainer decision on the issue thread, this widens act/aact to accept Component | CBlock | ModelOutputThunk and defaults their sampling strategy to None. instruct/chat/ainstruct keep their existing RejectionSamplingStrategy default — the change is scoped to act only.
Changes
Behavior note (async contract)
With strategy=None, an un-awaited aact now returns an uncomputed ModelOutputThunk for streaming (.value is None until awaited) — the least-surprise contract. Pass await_result=True to get a computed result. Two session tests were updated accordingly: test_aact now awaits its result, and the concurrent no-wait test was switched to SimpleContext (the documented context for parallel generation — ChatContext under asyncio.gather races on shared context). This race was previously masked by the old default strategy serializing the work.
Notice:
act/aact (both the
mellea.stdlib.functionalfunctions and theMelleaSessionmethods) now acceptComponent | CBlock | ModelOutputThunkas the action, and their default sampling strategy changes fromRejectionSamplingStrategy(loop_budget=2)toNone(single generation, no validation/repair loop). As a consequence, callers that relied on the implicit default while passingreturn_sampling_results=Truewill now raiseAssertionError(a strategy must be supplied explicitly), and callers passingrequirements=without an explicitstrategy=will have those requirements ignored with a warning rather than validated.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.