Skip to content

Split FinishedGraph off from GraphBuilder, return from worker#381

Open
acl-cqc wants to merge 28 commits intomainfrom
acl/finished-graph
Open

Split FinishedGraph off from GraphBuilder, return from worker#381
acl-cqc wants to merge 28 commits intomainfrom
acl/finished-graph

Conversation

@acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Mar 4, 2026

  • Add FinishedGraph holding a GraphData and outputs_type.
  • GraphBuilder.outputs renamed to finish_with_outputs and returns a FinishedGraph
  • Change methods that take GraphBuilders (e.g. loop, eval) to take FinishedGraph's, removing check in embed that the Output node is present (this is now guaranteed if your python code is well-typed - I was surprised not to find more such checks in map/loop/eval/const/etc.)
  • Add FinishedGraph as a PType (GraphData already is), and serialize FinishedGraphs as GraphDatas in the runtime. (That is, they are a build-time-only construct).
    • TODO: this could be done better, by defining a ModelConvertible or similar in types.py implemented by FinishedGraph (which then is used only in the builder and worker IDL).
  • Update stub generator to support TNamedModel's as well as PModel Structs (by adding an is_ptype field to GenericType - this is not super-neat, suggestions of better ways to do this welcome, but my previous attempt was even (much) worse]: 454c18f)
    • It'd be good to remove @runtime_checkable from the RestrictedNamedTuple subclasses/instantiations, but leaving that for Another PR
  • Update test functions, docs/notebooks, etc., and test worker (including tests from tests: more fun with workers making graphs #392) to take/return parametrized FinishedGraph rather than GraphData. This was a bit tedious, and the bulk of changed files in the PR; also suggests this might be the time to do the grand renaming in that I'm already writing e.g. workflow = g.finish_with_outputs(...)

The end result is that you need fewer TypedGraphRefs (but still some), and when you do need one, pyright should catch cases of providing the wrong outputs_type (because this is now just a runtime reification of a type that pyright knows statically).

Other possible TODOs (maybe too much for one PR??)

  • Remove output type from GraphBuilder (and provide when you set the outputs), adding a subclass RecursiveGraphBuilder that requires setting the output-type at the start (with fn graph_ref(self) moved into this Recursive variant)
  • Renaming could take place here: FinishedGraph -> Workflow, GraphBuilder -> Graph, finish_with_outputs -> finish/compile/into_workflow/???
  • We have quite a few tests that build GraphData's directly (not using the builder)....if we want the builder to be the only supported way of building graphs, we'll need to update these too.
    • If we do that, then this would allow hiding GraphData (i.e. making private, this is python) where it is used in the builder, so GraphData is really part of the graph runtime, not part of the user-facing graph-building API.

@acl-cqc acl-cqc force-pushed the acl/finished-graph branch from e8d272b to 7e92fcb Compare March 4, 2026 21:41
@acl-cqc acl-cqc changed the base branch from main to acl/tidy_types March 4, 2026 21:54
@acl-cqc acl-cqc force-pushed the acl/finished-graph branch 4 times, most recently from dc8ba1f to e8ce026 Compare March 4, 2026 22:08
@acl-cqc acl-cqc changed the title Split FinishedGraph off from GraphBuilder Split FinishedGraph off from GraphBuilder, return from worker Mar 4, 2026
"""
if isinstance(graph_data, GraphBuilder):
graph_data = graph_data.get_data()
if isinstance(graph_data, FinishedGraph):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has probably just broken visualization of half-built/in-progress graphs. I might need to take a GraphData | GraphBuilder | FinishedGraph here (and bring back some way to get_data the GraphData out of a GraphBuilder)

acl-cqc added a commit that referenced this pull request Mar 5, 2026
…puts_type before outputs_type (#391)

Also remove some overloads that seem pointless (??), and tidy some
imports

Removing generics_in_ptype as a preliminary to #381 because it's hard to
handle parametrized FinishedGraph.

`TypedGraphRef[Inputs, Outputs]` was constructed via
`TypedGraphRef(outputs_type, inputs_type)`, so reorder the latter.
(Although the `inputs_type` field is unused it helps `pyright` figure
out the `Inputs` type parameter when in strict mode or with
`reportUnknownVariableType` so keep it for now.)
Base automatically changed from acl/tidy_types to main March 5, 2026 17:49
@acl-cqc acl-cqc force-pushed the acl/finished-graph branch from e8ce026 to 969cd0f Compare March 5, 2026 22:31
acl-cqc added 2 commits March 5, 2026 22:45
…ke & stubs import FinGraph, regen stubs+import FinishedGraph...tests pass but pyright fails
@acl-cqc acl-cqc force-pushed the acl/finished-graph branch from 969cd0f to 66ec5c5 Compare March 5, 2026 22:46
acl-cqc added a commit that referenced this pull request Mar 9, 2026
These all work as things stand, but are intended as fodder for #381 -
they need a lot of manual typing with opportunities for error....
@acl-cqc acl-cqc force-pushed the acl/finished-graph branch from 25881e4 to 8c2ba50 Compare March 9, 2026 10:35

This will evaluate a nested graph with the given inputs.

:param A the input type of the graph.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I mean do we need these :types? Are they not inferred from the type annotations?

graph: GraphBuilder[A, B],
graph: FinishedGraph[A, B],
) -> TypedGraphRef[A, B]:
# TODO @philipp-seitz: Turn this into a public method?
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'd +1 this, shall I do it in this PR?

@acl-cqc acl-cqc requested a review from johnchildren March 9, 2026 14:37
@rossduncan
Copy link
Collaborator

Why do we need both FinishedGraph and GraphData ?

@acl-cqc
Copy link
Contributor Author

acl-cqc commented Mar 10, 2026

Why do we need both FinishedGraph and GraphData ?

As per last paragraph in the description (TODOs/future work):

  • We have quite a few tests that build GraphData's directly (not using the builder)....if we want the builder to be the only supported way of building graphs, we'll need to update these too.
    • If we do that, then this would allow hiding GraphData (i.e. making private, this is python) where it is used in the builder, so GraphData is really part of the graph runtime, not part of the user-facing graph-building API.

I.e. I think we need GraphData as an internal struct, for (a) serialization, and (b) passing from GraphBuilder into FinishedGraph, but we could hide it from the user with more updates to tests etc. Except this may restrict partial in the future, we should do a bit of investigation.

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