Skip to content

Extensions to graphcore#9

Open
jtoman wants to merge 6 commits intomasterfrom
jtoman/cvl-graphcore
Open

Extensions to graphcore#9
jtoman wants to merge 6 commits intomasterfrom
jtoman/cvl-graphcore

Conversation

@jtoman
Copy link
Contributor

@jtoman jtoman commented Feb 11, 2026

  1. Adds new "schema" classes that allow the implementation of a tool to be declared as a member of its schema.
  2. Adds "fluent" builder interface for graphs, providing an alternative to the clunky 1000 argument build_workflow function (and allowing efficient sharing for common graphs).
  3. Simplifies the type signatures of the result tool
  4. Adds a new "fs tools" which is simply for reading filesystems. This replaces the old (clunky) method of declaring a VFS state that is immutable and with no in-memory files.

* Use actual async implementation (haha woops)
* Make things optional to more resilient to the llm messing up
* Add fast path for just compiling the graph
@jtoman jtoman requested a review from certo-gco February 11, 2026 22:28
"""
Generic grep implementation over file contents.

Args:

Choose a reason for hiding this comment

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

Args in the doc string don't match the func's parameters.

def __call__(self, template_name: str, **kwargs: Any) -> str: ...


class Builder(
Copy link

@certo-gco certo-gco Feb 12, 2026

Choose a reason for hiding this comment

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

My understanding is that you plan to use this class as:

instance = Builder()
new_instance = instance.with_state(state)
# do something with new_instance
new_instance_with_context = new_instance.with_context(another_context)

If that's the case, new_instance_with_context would "inherit" the original new_instance's attributes by reference, due to _copy_{untyped,typed}_to_ which instantiate an empty Builder() and populate it by reference. This pattern does not preserve immutability across instances, which is what you want I suppose, that is, I guess you want new_instance_with_context to be a proxy of new_instance, with the former getting the attributes updates of the latter by reference.

Stated differently, with your current pattern (simplified):

builder1 = Builder(list, str, str).with_state(["foo", "bar"])
builder2 = builder1.with_context("some context")
builder1._state_class.append("bogus")
print(builder2._state_class)   # <-- this prints ["foo", "bar", "bogus"]

As a side note, with your current pattern, if you chain the methods "à la rust" like:

foo = Builder().with_state(state).with_context(context).with_input(input).build()

you'd instantiate 4 different objects, all of them eligible for GC, because in the end build only returns a Tuple of different objects.

My humble opinion is that you could get rid of the _copy_* pattern by simply doing something like:

def with_state(self, t: type[_BStateBind]) -> "Builder[_BStateBind, _BContextT, _BInputT]":
    self._state_class = t
    return self

if you don't need immutability. This would reduce the factory-like pattern boiler plate but also, more importantly, every time you add a new field, you don't need to remember to update these methods.

If instead you really intended to have immutable objects, then you could simply use copy for shallow copies or, better, dataclasses with .replace().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I do need immutability. there are many places where I build some "common" parameters of a builder and then use it multiple times to fill in the details. For example:

basic_builder = Builder().with_input(FlowInput).with_loader(load_jinja_template)

with_project_source = basic_builder.with_tools(fs_tools(...))

cvl_builder = basic_builder.with_tools([cvl_tools(...)]

I then use cvl_builder to build multiple workflows with different state types. Doing the changes by reference as you describe would be a disaster.

The reason for doing the explicit copying is that I believe it makes the type system happy. if I do copy or replace I don't think that let's me change the type parameters of the class I'm copying. I could always throw a type: ignore on there, but I prefer to avoid that where possible. Am I wrong?

Choose a reason for hiding this comment

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

Then in that case beware of the fact that Python objects (except for a few basic types such as strings and ints) are mutable by default. If a subset of builders inherit some attribute which is, say, dict-like or list-like, any modification to such attributes in basic_builder would propagate downstream (see my former example with builder1 and builder2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right. the only reference type I use is the tools list, which believe I am careful to copy.

Copy link

@certo-gco certo-gco left a comment

Choose a reason for hiding this comment

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

lgtm

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.

2 participants