tests: more fun with workers making graphs#392
Merged
Conversation
acl-cqc
commented
Mar 5, 2026
|
|
||
|
|
||
| class ApplyTwiceInput(NamedTuple): | ||
| # Note we mangle this as the stub generator does, unlike |
Contributor
Author
There was a problem hiding this comment.
This is a wee bit ugh, but seems correct for now, maybe we can improve it in #381
acl-cqc
commented
Mar 5, 2026
acl-cqc
commented
Mar 5, 2026
| just stubs-generate-new 'tierkreis_workers/pytket_worker' | ||
| just stubs-generate 'tierkreis_workers/quantinuum_worker' | ||
| just stubs-generate 'tierkreis_workers/qulacs_worker' | ||
| just stubs-generate 'tierkreis/tests/workers/graph' |
Contributor
Author
mwpb
approved these changes
Mar 6, 2026
5 tasks
johnchildren
approved these changes
Mar 9, 2026
johnchildren
pushed a commit
that referenced
this pull request
Mar 30, 2026
* Add parametrized `Workflow` holding a GraphData and outputs_type. * GraphBuilder renamed to Graph, and `.outputs` renamed to `.finish_with_outputs` which returns a Workflow - [ ] TODO: Rename `finish_with_outputs` to `build`?? * Change methods that take Graph(Builders) (e.g. `loop`, `eval`) to take Workflow'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 a `ModelConvertible` protocol like DictConvertible/etc. but that works for parametrized types. (This was a bit of a can of worms, there are some slightly suspect bits in `types.py`.) Workflow implements this, and serializes as a `GraphData`. (Type erasure: Workflow is a build-time-only construct). - [ ] This was in commit 6ccf080, previously I included Workflow in the union of `PType`'s which was a bit hacky (special-case, awkward circular import, etc.) but a lot shorter/easier (and `DictConvertible` makes me worry about robustness and parametrization). Thoughts? * Update stub generator to support TNamedModel's as well as PModel `Struct`s (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](#401) * Update test functions, docs/notebooks, etc., and test worker (including tests from #392) to take/return parametrized Workflow rather than GraphData. This was a bit tedious, and the bulk of changed files in the PR... I have tried to update the text of the docs a little but there is a lot of use of the word "graph" to mean either an unfinished one or a finished one, and trying to distinguish which of those should be workflows is...tricky. This may suggest calling the two classes `GraphBuilder` and `Graph` rather than `Graph` and `Workflow`??? The end result is that you need fewer `TypedGraphRef`s (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 Graph (and provide when you set the outputs)? This would mean splitting the class hierarchy i.e. we'd need a parent `_BaseGraph[Inputs]` with all methods *except* `finish_with_outputs` and `graph_ref`, then `class Graph[Inputs](_BaseGraph[Inputs]): ...fn finish_with_outputs[Outputs](self, outputs_type: type[Outputs], outputs: Outputs)` and separately `class RecursiveGraph[Inputs,Outputs](_BaseGraph[Inputs]): ...fn finish_with_outputs(self, outputs: Outputs)` plus `fn graph_ref(self)`. Not sure whether this is worth it so I think leave for another PR? - [ ] 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These all work as things stand, but are intended as fodder for #381 - they need a lot of manual typing with opportunities for error....