feat: preserve Context subtype and enforce input==output context type - #1582
feat: preserve Context subtype and enforce input==output context type#1582AngeloDanducci wants to merge 5 commits into
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
jakelorocco
left a comment
There was a problem hiding this comment.
looks good; I have a few concerns / thoughts that I think would be worth addressing in this PR so that we can get a better surface for the context typing.
…verride Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
| for sample_ctx in sampling_result.sample_contexts: | ||
| _enforce_context_type( | ||
| context, | ||
| sample_ctx, | ||
| allow_context_type_change=allow_context_type_change, | ||
| ) |
There was a problem hiding this comment.
Can you please add new tests for this as well?
| ) | ||
|
|
||
|
|
||
| class Context(abc.ABC): |
There was a problem hiding this comment.
I looked at the implementation for our existing contexts and I think they will require fixes here as well. It looks like several of their methods use the named class constructor instead of self, etc... which will cause subclasses to fail.
There was a problem hiding this comment.
Rechecked the constructor paths at b4ca7bce. Using type(self) fixes bare-subclass demotion, but subclasses with required constructor arguments now fail: ChatContext._make_root() calls type(self)(), while both built-in add() paths reach Context.from_previous() → cls(). A subclass with __init__(self, tag: str) raises TypeError on its first add(); ChatContext also fails through new_instance() during reset/model binding.
Please either preserve subtype state without re-running subclass initialisers, including the Context.from_previous() path, or explicitly require subclasses to be constructible with no arguments. Add a required-argument-subclass regression test for the chosen contract.
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
planetf1
left a comment
There was a problem hiding this comment.
I found a few API, typing, and lifecycle issues inline. The new runtime subtype coverage and the checks over every sampling context are both useful additions.
| return sampling_result | ||
| else: | ||
| return result, new_ctx | ||
| checked_ctx = _enforce_context_type( |
There was a problem hiding this comment.
One lifecycle ordering issue: the context guard runs after component_post_success. When it rejects new_ctx, the exception handler then emits component_post_error, so plugins see both terminal events. I reproduced ['success', 'ContextTypeMismatchError']; the tracing plugin closes the action span as successful before the error hook runs. Could we validate the returned context(s) before constructing the success payload, so a rejected call emits only the error path?
| ContextTypeMismatchError: If the output context type differs from the | ||
| input context type and `allow_context_type_change` is `False`. | ||
| """ | ||
| if type(output_ctx) is type(input_ctx) or allow_context_type_change: |
There was a problem hiding this comment.
This escape hatch is runtime-correct but statically unsound. With allow_context_type_change=True, a ChatContext can become a SimpleContext, yet this cast and all public overloads still return ContextT. That also leaves MelleaSession[ChatContext].ctx typed as ChatContext after the switch, so session.ctx.model_id type-checks even though the runtime object has no such attribute. Could the Literal[True] overloads widen to Context, with sessions that permit switching represented as MelleaSession[Context]?
There was a problem hiding this comment.
Widened this, could use another set of eyes.
There was a problem hiding this comment.
Rechecked at b4ca7bce. The literal-True widening is in place for the existing functions, but three gaps remain:
achathas no overloads, soallow_context_type_change=Truestill returnstuple[Message, ChatContext]rather than widening the context. Please mirrorchatand add a typing assertion.MelleaSession(..., allow_context_type_change=True)remainsMelleaSession[ChatContext], unlikestart_session, which widens toMelleaSession[Context].- A runtime
boolmatches neitherLiteraloverload. This leaves nine suppressed internal calls inferred asAny, and makesstart_session(ctx=..., allow_context_type_change=flag)a publiccall-overloaderror. Addbool → Contextfallback overloads for each applicable non-SamplingResultreturn shape and thectx-suppliedstart_sessionforms.
|
|
||
| @abc.abstractmethod | ||
| def add(self, c: Span) -> Context: | ||
| def add(self, c: Span) -> Self: |
There was a problem hiding this comment.
Changing this public abstract method from Context to Self is a source-compatible runtime change, but it breaks typed third-party subclasses: an existing def add(...) -> Context now fails mypy's override check. Is that breaking API change intentional? If not, it may be safer to retain the abstract Context return and provide the narrower Self types on the concrete built-in contexts.
There was a problem hiding this comment.
should now narrow to self, the return while the abstract
signature stays Context for source-compat
| node._model_id = model_id | ||
|
|
||
| ctx: ChatContext = ChatContext.__new__(ChatContext) | ||
| ctx: ChatContext = cls.__new__(cls) |
There was a problem hiding this comment.
Preserving the subclass here bypasses its initializer and restores only the three built-in fields. That conflicts with _propagated_fields, whose class documentation invites subclasses to add state there. I reproduced a subclass-owned field disappearing after window compaction, then raising AttributeError on the next add(); a subclass with a required constructor argument already fails on the first add() through type(self)(). This needs a subclass-aware factory or clone/rebuild hook rather than cls.__new__().
There was a problem hiding this comment.
should now properly propagate
There was a problem hiding this comment.
The rebuild now preserves the concrete subtype, but the new compaction test only checks type identity. The original regression was a subclass-owned _propagated_fields value being discarded. Please extend it with a ChatContext subclass that registers an extra field, trigger compaction, and assert that field survives on the rebuilt context.
| new = ChatContext.from_previous(self, c) | ||
| # `type(self)`, not `ChatContext`, so a subclass gets an instance of | ||
| # itself back rather than being silently demoted to `ChatContext`. | ||
| new = type(self).from_previous(self, c) |
There was a problem hiding this comment.
The new runtime subclass checks are helpful. There is still a static half to the same contract: add() is annotated as returning ChatContext, and SimpleContext.add() returns SimpleContext. Mypy therefore widens a bare subclass at its first add(), despite the runtime object retaining its subtype. Could these use Self (and the compactor path preserve that type), with subclass assert_type checks added alongside the runtime tests?
There was a problem hiding this comment.
ChatContext.add and SimpleContext.add now return Self, and the
compactor path preserves it
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
| return cls() | ||
|
|
||
| def new_instance(self) -> Context: | ||
| def new_instance(self) -> Self: |
There was a problem hiding this comment.
Separate source-compatibility issue on the sibling factory: Context.new_instance() now returns Self. A third-party override returning Context—the previous base contract, and one the docstring invites—now fails mypy’s override check.
Please keep the base return type as Context, matching add(). ChatContext.new_instance() can narrow its own return to Self, preserving subtype inference without breaking existing overrides.
| await_result: bool = False, | ||
| ) -> tuple[ModelOutputThunk[S], Context] | SamplingResult: | ||
| ) -> tuple[ModelOutputThunk[S], ContextT] | SamplingResult: | ||
| """Asynchronous version of .act; runs a generic action, and adds both the action and the result to the context. |
There was a problem hiding this comment.
Please document the sampling-context validation scope. With return_sampling_results=False, only the chosen result_ctx is returned and checked; with True, every sample_contexts entry is returned and validated. That distinction is coherent, but it is surprising enough to deserve one sentence in the public API documentation.
| def _rebuild_chat_context( | ||
| components: list[Span], | ||
| *, | ||
| source: ChatContext, |
There was a problem hiding this comment.
Was this compatibility break intentional? The documented custom-compactor recipe imports this underscore-private helper directly, so copied versions of the old call shape now fail because source is required; None for the configuration arguments also now inherits from source instead of clearing the field.
If intentional, please note the migration. If not, preserve the previous call shape with a source=None fallback.
planetf1
left a comment
There was a problem hiding this comment.
Re-review at b4ca7bce: the previous fixes hold. Requesting changes for the five remaining issues called out inline: the incomplete literal/runtime-bool overload contract, required-argument context subclasses, and new_instance() source compatibility. The sampling-scope and compactor-compatibility comments are non-blocking.
Required CI is green at this head.
Pull Request
Issue
Fixes #1522
Description
preserve Context subtype and enforce input==output context type
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.