Skip to content

Conversation

@HendrikStrobelt
Copy link
Contributor

No description provided.

Context -> LegacyContext
*Context -> Legacy*Context
@mergify
Copy link

mergify bot commented Sep 25, 2025

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?:

Comment on lines 359 to 360
self._previous = RootContext()
self._data = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how I feel about having a specific type of RootContext. I think if self._previous is None, you can assume it's the root of a context. This also lets you abruptly truncate contexts, ie you can detach a child from its parent just by setting self._previous = None.

If we want to store context level things in a root node (like common docs, common vars, etc...), then I think it potentially makes sense to have a specific root node class that handles those things. But I haven't really thought out those design decisions yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with using "None". I was just inspired by other libraries which also use own keywords instead of reserved keywords for neutral values like NoValue,....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using RootContext would avoid adding if conditionals at every possible occasion that it could occur, because calling any method on None raises an error. RootContext can have its methods that act transparently and thus is superior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, RootContext.linearize would just return self.

Comment on lines 363 to 381
def from_previous(cls, previous: Context, data: Component | CBlock) -> Context:
"""Constructs a new context from an existing context."""

assert isinstance(previous, Context), (
"Cannot create a new context from a non-Context object."
)
assert data is not None, "Cannot create a new context from None data."

# if previous is an empty root context, create a new root context with the data.
if previous._data is None and isinstance(previous._previous, RootContext):
x = cls()
x._previous = RootContext()
x._data = data
return x
else:
x = cls()
x._previous = previous
x._data = data
return x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic is a bit weird. It's basically saying the current context node is the root if it has no data and a previous pointer to a RootContext. I think if we have a RootContext object, all context nodes under it should have data.

This also causes a weird issue with extend / from_previous. I think the assumption is that expanding a context should always add a new node to the existing context tree. For example,

ctx = Context() # node1(empty)
ctx2 = ctx.expand(CBlock("hello")) # node1(empty) -> node2("hello")

Instead what we currently get is something like:

ctx = Context() # RootContext -> node1(empty)
ctx2 = ctx.expand(CBlock("Hello")) # RootContext -> node2("hello")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you now spontaneously disconnect, it's pretty inconsistent imho:

ctx = Context() # node1(empty)
ctx2 = ctx.expand(CBlock("hello")) # node1(empty) -> node2("hello")
ctx3 = ctx.expand(CBlock("world")) # node1(empty) -> node2("hello") -> node3("world")

disconnect at 2 leads to:

node2("hello") -> node3("world")

which is different in schema than ctx2

return []


class LegacyContext(abc.ABC):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to keep around the old contexts? I feel like we might almost want to keep the naming of the old contexts, make sure the user facing interfaces still work (like get_last_prompt), and basically pretend like nothing happened. That way it's not a breaking change and people likely don't need to change their code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is to keep everything running and piece by piece replacing LegacyContext with Context, but do it carefully and intentionally. Latest at the last commit in this PR there should be no trace of Legacy* anymore.

"""A `RootContext` is a special type of `Context` that is empty and is used to represent the root context of a `MelleaSession`."""


class Context(abc.ABC):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a note here in case we want to add the children field as well:

  • Appending to a list in python should be atomic, so we wouldn't need a lock for that (since we don't care about order either)
  • Removing objects from a list is not, but I don't think we actually need to do that

return x

@abc.abstractmethod
def expand(self, c: Component | CBlock) -> Context:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.add()

"""Returns the data associated with this context."""
return self._data

def full_data_as_list(self) -> list[Component | CBlock]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is for documentation of decision making.

@jakelorocco comments as_list is a better name because full or complete entails that you are getting the entire tree structure (which you are not). I agree.

But @HendrikStrobelt is giving a more verbose function name here because this function does NOT respect the view.

Aside: Nathan's Proposal

Nathan's Proposal: we separate the functionality of a Context from the functionality of a ContextView on that context.

The protocol for Context contains:

Context:
    add : Context * Component -> Context
    data_as_list : Context -> List[Component | CBlock]
    data : Context -> Component | CBlock | None

The protocol for ContextView contains:

ContextView:
    view : Context -> List[Component | CBlock]?

We then still have the same ergonomics:

class LinearContext(Context, ContextView)

But implementation of new views are forced to implement their functionality in terms of the Context interface.

Both Hendrik and Jake disagree with Nathan's Context / ContextView proposal :)

TODOs

  • Change this function name to as_list : Context * int -> list[Component | CBlock], and add a prominent comment explaining that the view constraints are NOT respected by this function.

HendrikStrobelt and others added 10 commits September 26, 2025 11:48
ctx.as_list(last_n_components..)

ctx.render_for_generation() -->
ctx.view_for_generation()
2) making rejection-sampling default
- all: convert to new cotext
- contextual_session test is commented out with TODO
- session_ctx test is removed because logging changed.
@HendrikStrobelt HendrikStrobelt marked this pull request as ready for review September 26, 2025 21:39
@HendrikStrobelt
Copy link
Contributor Author

approved by slack

@HendrikStrobelt HendrikStrobelt merged commit 4ae6d7c into main Sep 29, 2025
4 checks passed
@HendrikStrobelt HendrikStrobelt deleted the all/m_v7 branch September 29, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants