from_dot feature #1489

Krishn1412 · 2025-08-09T21:28:15Z

Feat: Add from_dot parser support

Implement DOT file parsing to load graph structures
Support reading DOT content from string input
Added grammar using pest
I ran the rust formatter and clippy
I have added test coverage for the changes

Built a basic grammar and wired it up with functions to parse it and added it in the pymodule. Next steps: 1) Fix the grammar, make it robust. 2) Add tests and documentations

IvanIsCoding

This is an excellent start!

I will give it a deeper review, specially about graphviz's dot grammar which I need to understand more deeply.

I left some comments about tests and the grammar to get started

tests/graph/test_dot.py

src/dot_parser/dot.pest

IvanIsCoding · 2025-08-10T21:43:37Z

src/dot_parser/mod.rs

+                    }
+
+                    Rule::subgraph => {
+                        // TODO: subgraph handling


It's ok to return an error for subgraphs now. dot might be too expresive. We just want to make sure the output from a rustworkx entry can be re-imported as the original goal.

Once we have that, we can support more advanced features

IvanIsCoding · 2025-08-10T23:28:41Z

src/dot_parser/mod.rs

+            });
+        } else {
+            let graph = StablePyGraph::<Undirected>::with_capacity(0, 0);
+            undi_graph = Some(PyGraph {


Some thoughts about this handling:

I'd make a function that is parametrized on StablePyGraph e.g.

rustworkx/src/union.rs

Line 37 in f0240d4

fn union<Ty: EdgeType>(

Upon detecting the type, call the parametrization on the if/else

If you need to detect the direction of the graph, call https://docs.rs/petgraph/latest/petgraph/stable_graph/struct.StableGraph.html#method.is_directed within the parametrized function.

I think that choice will simplify your code, especially for handling both di_graph and undi_graph on every statement

IvanIsCoding · 2025-08-10T23:31:23Z

src/dot_parser/mod.rs

+                            let src_idx = *node_map.entry(src.clone()).or_insert_with(|| {
+                                let py_node = PyString::new(py, &src).into();
+                                if is_directed {
+                                    di_graph.as_mut().unwrap().graph.add_node(py_node)


If you follow the previous recommendation, this becomes graph.add_node which arguably is simpler that handling the Option<PyGraph> each time

1) Added a new function to use stable graph with is_directed as a parameter to avoid creating python object every time. 2) Added more tests to include round trip from to_dot and from_dot for graph and digraph.

Krishn1412 · 2025-09-06T17:10:19Z

Hey @IvanIsCoding, could you take a look again.

IvanIsCoding

I left some minor comments, but this is very close to getting merged:

Use the hashmap from hashbrown for consistency. It should be slightly faster because of the hasher it uses vs std which uses a more conservative hasher
Avoid panicking, be careful with unwrap(). Python users cannot recover easily from panic, they'd rather deal with exceptions.

And last but not least, don't forget to add a release note: https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#release-notes. I am planning on promoting this as one of the key new features in the 0.18.0 launch. People will want to read about it, it's handy!

IvanIsCoding · 2025-09-07T17:00:44Z

src/dot_parser/mod.rs

+use crate::StablePyGraph;
+
+use rustworkx_core::petgraph::prelude::{Directed, NodeIndex, Undirected};
+use std::collections::HashMap;


Use the hashmap from hashbrown: https://docs.rs/hashbrown/latest/hashbrown/struct.HashMap.html

That's what we use everywhere

src/dot_parser/dot.pest

IvanIsCoding · 2025-09-07T17:02:21Z

src/dot_parser/mod.rs

+            continue;
+        }
+        let mut inner = pair.into_inner();
+        let first = inner.next().unwrap();


Let's avoid unwrapping if possible, do inner.next()?.

IvanIsCoding · 2025-09-07T17:02:57Z

src/dot_parser/mod.rs

+                match stmt.as_rule() {
+                    Rule::node_stmt => {
+                        let mut it = stmt.into_inner();
+                        let nid = it.next().unwrap();


Same comment here. If you keep needing a conversion from pest errors to PyResult, make a helper closure and do a .map_err or something along those lines

IvanIsCoding · 2025-09-07T17:03:42Z

src/dot_parser/mod.rs

+
+                    Rule::assignment => {
+                        let mut parts = stmt.into_inner();
+                        let key = parts.next().map(|p| p.as_str()).unwrap_or("");


This unwrap_or is fine. Keep it as is.

IvanIsCoding

Also, I think you'll need to downgrade pest to compile with our MSRV.

I can also bump the MSRV in a separate PR

IvanIsCoding · 2025-09-07T22:25:55Z

Cargo.toml

@@ -64,6 +64,8 @@ serde_json = "1.0"
 smallvec = { version = "1.0", features = ["union"] }
 rustworkx-core = { path = "rustworkx-core", version = "=0.17.0" }
 flate2 = "1.0.35"
+pest = "2.7"


By the way, changes to Cargo.toml should also have a corresponding Cargo.lock change. Did you forget to commit Cargo.lock?

Krishn1412 and others added 4 commits August 2, 2025 17:02

First draft

d11ce53

Built a basic grammar and wired it up with functions to parse it and added it in the pymodule. Next steps: 1) Fix the grammar, make it robust. 2) Add tests and documentations

Updating grammar

06273db

from_dot feature

7d849de

Merge branch 'main' into from_dot_feature

ae8e199

Krishn1412 mentioned this pull request Aug 10, 2025

Add from_dot to create rustworkx graphs from dot files #1466

Open

IvanIsCoding reviewed Aug 10, 2025

View reviewed changes

tests/graph/test_dot.py Show resolved Hide resolved

src/dot_parser/dot.pest Show resolved Hide resolved

IvanIsCoding reviewed Aug 10, 2025

View reviewed changes

Krishn1412 added 2 commits August 23, 2025 23:47

Updates based on comments

a40b782

1) Added a new function to use stable graph with is_directed as a parameter to avoid creating python object every time. 2) Added more tests to include round trip from to_dot and from_dot for graph and digraph.

fixing clippy

2ac3ec1

IvanIsCoding reviewed Sep 7, 2025

View reviewed changes

Updates based on review

dbe10e7

Krishn1412 force-pushed the from_dot_feature branch from 8232427 to dbe10e7 Compare September 11, 2025 18:28

Merge branch 'main' into from_dot_feature

38ae858

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

from_dot feature #1489

from_dot feature #1489

Krishn1412 commented Aug 9, 2025

Uh oh!

IvanIsCoding left a comment

Uh oh!

Uh oh!

Uh oh!

IvanIsCoding Aug 10, 2025

Uh oh!

IvanIsCoding Aug 10, 2025

Uh oh!

IvanIsCoding Aug 10, 2025

Uh oh!

Krishn1412 commented Sep 6, 2025

Uh oh!

IvanIsCoding left a comment

Uh oh!

IvanIsCoding Sep 7, 2025

Uh oh!

Uh oh!

IvanIsCoding Sep 7, 2025

Uh oh!

IvanIsCoding Sep 7, 2025

Uh oh!

IvanIsCoding Sep 7, 2025

Uh oh!

IvanIsCoding left a comment

Uh oh!

IvanIsCoding Sep 7, 2025

Uh oh!

Uh oh!

from_dot feature #1489

Are you sure you want to change the base?

from_dot feature #1489

Conversation

Krishn1412 commented Aug 9, 2025

Uh oh!

IvanIsCoding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krishn1412 commented Sep 6, 2025

Uh oh!

IvanIsCoding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanIsCoding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!