prototype walker #626

doug-q · 2023-10-26T08:30:01Z

No description provided.

doug-q · 2023-10-30T09:59:07Z

Needs docs. Feel free to suggest better names.

acl-cqc

I love this stack of pre/post visit WorkItems! :-). I think my biggest concern is with CFG nodes - I gather that is the intention, so is the idea to walk only the single basic block inside the CFG inside the function definition that get from Guppy, or what?

acl-cqc · 2023-10-31T15:42:04Z

src/walker.rs

+
+    pub fn walk(&mut self, mut t: T) -> Result<T, E> {
+        // We intentionally avoid recursion so that we can robustly accept very deep hugrs
+        let mut worklist = vec![WorkItem::Visit(self.hugr.root())];


Can you actually put the WorkItem struct in here?

Do you mean the definition? that would be better.

acl-cqc · 2023-10-31T15:45:42Z

src/walker.rs

+                    // the input node. (For example, LoadConstant nodes may not
+                    // be reachable from the input node). So we do a second
+                    // unordered traversal afterwards.
+                    if let Some([input, _]) = self.hugr.get_io(n) {


I am a little concerned that cfg nodes appear to be just skipped here. IIUC, guppy (for example) produces Hugrs where each function's body is a CFG (perhaps containing only a single basic block). Obviously "topological sort order" may not be viable for a CFG, but dfs postorder is....

One might want to handle Module nodes too, so you really can start this off for any Hugr; although that is maybe less pressing, as there is no chance of finding a Module nested within a DFG (whereas you might find CFGs/DFGs arbitrarily nested).

CFG and Module nodes work fine, they are caught be the second traversal. There is a test on Modules.

We do a (reverse) PostDFS traversal, which will visit nodes in topological order where this makes sense (when the sibling graph has an Input node), then a second traversal that catches anything else i.e. BasicBlocks or FuncDefns

Ah I see got it. I hadn't lined up the indenting correctly / realized the closing }. In that case this all looks good, but would be good to mention in the comments

Maybe just take "So we do a second unordered traversal aftewards", put on a new line, and add "this covers non-dataflow sibling graphs too"?

src/walker.rs

acl-cqc · 2023-10-31T15:51:14Z

src/walker.rs

+                    // We intend to only visit direct children.
+                    //
+                    // If the nodes children form a dataflow sibling graph we
+                    // visit them in post dfs order. This traversal is not


"Post DFS order" is a reverse topological sort? i.e. users of a value produced, before the node produces a value?

I think it'd be good to describe in a comment the order in which Pre/Post walkers will see nodes i.e. how this relates to a topological sort

The only other place I'm aware of where we use petgraph walkers is validate_children_dag (validate.rs) - where we use a Topo. That definitely breaks for CFGs, but for DFGs appears to do in one pass what you're having to do two passes for here?

Alternatively/however, you might want to generalize this to CFGs too - see comment below

I agree a better comment is needed. Post DFS is reverse topological sort, but we push them to a stack so they are visited in reverse Post DFS order.

src/walker.rs

acl-cqc · 2023-10-31T15:58:57Z

src/walker.rs

+        self
+    }
+
+    pub fn walk(&mut self, mut t: T) -> Result<T, E> {


Why not pass the Hugr(View) in here and remove self.hugr?

Because the callbacks need access to the hugr. In the current design the hugr is fixed for a given Walker, so the callbacks can capture the hugr. Passing the hugr here would mean that the Walker could be invoked on many hugrs, so the callbacks couldn't capture it. The hugr would need to be threaded through the callbacks. This can be done but I think it's less ergonomic.

acl-cqc · 2023-10-31T16:00:22Z

src/walker.rs

+                    for cb in self.mut_callbacks(order).iter_mut() {
+                        // this clone is unfortunate, to avoid this we would
+                        // need a TryInto variant like:
+                        // try_into(&O) -> Option<&T>


I bet there is some way with enum_dispatch but I'll leave that to someone Rustier than me.... @aborgna-q ? ;)

A clone is no big deal, I'm happy with that

Co-authored-by: Alan Lawrence <[email protected]>

ss2165

Looks good but my main question is why the behaviour is "walk the hugr, and visit nodes of a certain type, doing something to some accumulated data when you visit" (if I've understood correctly).

It seems more useful in general to do "walk the hugr, returning an iterator" and then you can call a normal filter or whatever on the iterator, and for callbacks a more general input would be just the node (maybe the + an immutable hugrview?) and I can make arbitrary querires on the hugr to decide what to do to the accumulated data, like queries about connectivity that I can't get from just the node weight.

I might be missing something about the functionality here - if so it would be nice to have tests which demonstrate the kinds of use case I'm talking about. (Suggested test - walk the hugr and record a hashmap keyed by node, only for LeafOps, with the value being the degree of the node, including counting each edge of a multiport).

doug-q · 2023-11-02T09:03:05Z

Looks good but my main question is why the behaviour is "walk the hugr, and visit nodes of a certain type, doing something to some accumulated data when you visit" (if I've understood correctly).

The behaviour is this way so that one has the option of maintaining a context as you descend/ascend the heirarchy. See the new pretty_printer test where the indent level is increased once for each level of nesting.

It seems more useful in general to do "walk the hugr, returning an iterator" and then you can call a normal filter or whatever on the iterator, and for callbacks a more general input would be just the node (maybe the + an immutable hugrview?) and I can make arbitrary querires on the hugr to decide what to do to the accumulated data, like queries about connectivity that I can't get from just the node weight.

I had thought that this was general enough to implement that iterator, but on reflection (and see hugr_walk_iter()) this is not good enough, because it's not lazy. walk should be changed to return an Iterator and the implementation of walk moved into that Iterator implementation.

I've changed the general form of callback to not take the OpType. I've not changed the signature of visit, the

I might be missing something about the functionality here - if so it would be nice to have tests which demonstrate the kinds of use case I'm talking about. (Suggested test - walk the hugr and record a hashmap keyed by node, only for LeafOps, with the value being the degree of the node, including counting each edge of a multiport).

I've added this test(mostly, I need to add an actual hugr for it to run on), as well as a pretty printing test that shows the usefulness of maintaining context as you traverse the heirarchy. I've also added a find function, demonstrating early return.

doug-q · 2024-02-05T13:35:40Z

Closing in favour of #830

Bumps [pytest-cov](https://github.com/pytest-dev/pytest-cov) from 4.1.0 to 5.0.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst">pytest-cov's changelog</a>.</em></p> <blockquote> <h2>5.0.0 (2024-03-24)</h2> <ul> <li>Removed support for xdist rsync (now deprecated). Contributed by Matthias Reichenbach in <code>[#623](pytest-dev/pytest-cov#623) <https://github.com/pytest-dev/pytest-cov/pull/623></code>_.</li> <li>Switched docs theme to Furo.</li> <li>Various legacy Python cleanup and CI improvements. Contributed by Christian Clauss and Hugo van Kemenade in <code>[#630](pytest-dev/pytest-cov#630) <https://github.com/pytest-dev/pytest-cov/pull/630></code><em>, <code>[#631](pytest-dev/pytest-cov#631) <https://github.com/pytest-dev/pytest-cov/pull/631></code></em>, <code>[#632](pytest-dev/pytest-cov#632) <https://github.com/pytest-dev/pytest-cov/pull/632></code>_ and <code>[#633](pytest-dev/pytest-cov#633) <https://github.com/pytest-dev/pytest-cov/pull/633></code>_.</li> <li>Added a <code>pyproject.toml</code> example in the docs. Contributed by Dawn James in <code>[#626](pytest-dev/pytest-cov#626) <https://github.com/pytest-dev/pytest-cov/pull/626></code>_.</li> <li>Modernized project's pre-commit hooks to use ruff. Initial POC contributed by Christian Clauss in <code>[#584](pytest-dev/pytest-cov#584) <https://github.com/pytest-dev/pytest-cov/pull/584></code>_.</li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/5295ce01c84262cec88f31255e9ac538718f3047"><code>5295ce0</code></a> Bump version: 4.1.0 → 5.0.0</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/1181b067972bf94569f8011f3b18f271690f9ab1"><code>1181b06</code></a> Update changelog.</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/9757222e2e044361e70125ebdd96e5eb87395983"><code>9757222</code></a> Fix a minor grammar error (<a href="https://redirect.github.com/pytest-dev/pytest-cov/issues/636">#636</a>)</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/9f5cd81a0dbe3fe41681efdbef516c08988fe8ff"><code>9f5cd81</code></a> Cleanup releasing instructions. Closes <a href="https://redirect.github.com/pytest-dev/pytest-cov/issues/616">#616</a>.</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/93b5047ec5050d63c10a6fe16a09b671a7a03df8"><code>93b5047</code></a> Add test for pyproject.toml loading without explicit --cov-config. Ref <a href="https://redirect.github.com/pytest-dev/pytest-cov/issues/508">#508</a>.</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/ff50860d7c67b920503745d92a3f0944cf41f982"><code>ff50860</code></a> docs: add config instructions for pyproject.toml.</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/4a5a4b5fa4b1c63ddcab5cbc1813798c9b6f1d36"><code>4a5a4b5</code></a> Keep GitHub Actions up to date with GitHub's Dependabot</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/1d7f55963d5138f41c452a946f7cca7e0b6ee8b2"><code>1d7f559</code></a> Fix or remove URLs that are causing docs tests to fail</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/6a5af8e85b8242ac815f33e26adf9068f5f0ebc3"><code>6a5af8e</code></a> Update changelog.</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/d9fe8dfed15023d3410dd299c5092e755b8981c2"><code>d9fe8df</code></a> Switch to furo. Closes <a href="https://redirect.github.com/pytest-dev/pytest-cov/issues/618">#618</a>.</li> <li>Additional commits viewable in <a href="https://github.com/pytest-dev/pytest-cov/compare/v4.1.0...v5.0.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pytest-cov&package-manager=pip&previous-version=4.1.0&new-version=5.0.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

doug-q added 2 commits October 26, 2023 09:29

prototype walker

bb76660

fix walker, rustfmt

6b7615c

doug-q force-pushed the feat/walker branch from adba209 to 6b7615c Compare October 26, 2023 11:36

tidying

27503f4

doug-q requested review from ss2165 and acl-cqc October 30, 2023 09:58

doug-q added 3 commits October 30, 2023 13:27

derive Deref/DerefMut

27f2595

Traverse in topological order when possible

fcc78d4

Callbacks need to reference the hugr, so ensure it is fixed in new

9973b72

acl-cqc reviewed Oct 31, 2023

View reviewed changes

Update src/walker.rs

4134536

Co-authored-by: Alan Lawrence <[email protected]>

ss2165 reviewed Oct 31, 2023

View reviewed changes

doug-q added 3 commits November 2, 2023 08:10

tidy

e4cc9eb

better comments, more examples

9f2265c

General callbacks don't need optype

c91a025

ss2165 mentioned this pull request Nov 15, 2023

feat: automatic mermaid diagram generation #696

Closed

doug-q closed this Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prototype walker #626

prototype walker #626

doug-q commented Oct 26, 2023

doug-q commented Oct 30, 2023

acl-cqc left a comment

acl-cqc Oct 31, 2023

doug-q Oct 31, 2023

acl-cqc Oct 31, 2023

acl-cqc Oct 31, 2023

doug-q Oct 31, 2023 •

edited

Loading

acl-cqc Nov 1, 2023

acl-cqc Nov 1, 2023

acl-cqc Oct 31, 2023

doug-q Oct 31, 2023

acl-cqc Oct 31, 2023

doug-q Oct 31, 2023

acl-cqc Oct 31, 2023

ss2165 left a comment

doug-q commented Nov 2, 2023

doug-q commented Feb 5, 2024

prototype walker #626

prototype walker #626

Conversation

doug-q commented Oct 26, 2023

doug-q commented Oct 30, 2023

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-q Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ss2165 left a comment

Choose a reason for hiding this comment

doug-q commented Nov 2, 2023

doug-q commented Feb 5, 2024

doug-q Oct 31, 2023 •

edited

Loading