Potential fix to the tooltip with special characters bug #1203

anushkrishnav · 2024-05-30T10:33:47Z

Fixes #750

Updated the fn attr_map_to_string function
I can add a separate function to handle the escape values if needed

fn escape_graphviz_value(value: &str) -> String {
    value
        .replace("\\", "\\\\")  // Escape backslashes first
        .replace("\"", "\\\"")  // Escape double quotes
        .replace("\n", "\\n")   // Escape newlines
        .replace(":", "\\:")    // Escape colons
}

Will finish up docs and tests once the solution is satisfactory

I ran rustfmt locally
I have added the tests to cover my changes.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

CLAassistant · 2024-05-30T10:33:53Z

All committers have signed the CLA.

mtreinish

Did you push the correct branch in your PR? The code change here is adding a print statement (it looks like a debug print) and not fixing the bug.

anushkrishnav · 2024-05-31T10:19:04Z

Did you push the correct branch in your PR? The code change here is adding a print statement (it looks like a debug print) and not fixing the bug.

ohh sorry Let me check whats happening,

anushkrishnav · 2024-05-31T10:26:11Z

@mtreinish Sorry about the print statement , is this how the output must look like ?

it would be saved as a graph.svg ??

I am sorry looks like i forgot to push the update , Silly

IvanIsCoding

I think the idea is good, but we definetely need to check if we covered all possible characters that need escaping. And add tests

src/dot_utils.rs

anushkrishnav · 2024-06-01T15:56:06Z

I think the idea is good, but we definitely need to check if we covered all possible characters that need escaping. And add tests

I belive using a package like serde json should be a better way to do it that building a dictionary of cases that we cant escaped

anushkrishnav · 2024-06-01T16:03:10Z

I think the idea is good, but we definetely need to check if we covered all possible characters that need escaping. And add tests

Yes We can add a custom test case that tests for combinations of escape sequences
@IvanIsCoding please check and let me know if its good to go

IvanIsCoding · 2024-06-04T02:44:00Z

tests/visualization/test_graphviz.py

+            return {"label": label, "tooltip": tooltip}
+
+        # Draw the graph using graphviz_draw
+        image = graphviz_draw(


I think it would be better to test with https://www.rustworkx.org/dev/apiref/rustworkx.PyGraph.to_dot.html

sounds good will work on em

@IvanIsCoding Do you want me to run the code on the site or ?

I want you to avoid writing temporary files to the disk for the unit tests. For the GitHub CI, we found those tests very flaky on macOS. So I suggested that method as a way of testing the fix with only in-memory operations

Sounds good, Update the code with the recommendation, let me know if it needs any changes

tests/visualization/test_graphviz.py

IvanIsCoding · 2024-06-06T12:11:48Z

Rust code is looking good, just two minor things and this will be good to go:

Add release notes with a graph exemplifying the fix.

rustworkx/releasenotes/notes/0.14/add-albert-graph-5a7d393e1fe18e9d.yaml

Line 9 in 53868f8

.. jupyter-execute::

is a good reference, but you will need to use graphviz_draw instead of mpl_draw, of course.
Address the Clippy fail in CI

IvanIsCoding

@anushkrishnav I approved the CL but you need to fix the CI errors by running black, cargo clippy. I also recommend running nox -edocs to confirm that the documentation compiles too

anushkrishnav · 2024-06-08T04:51:22Z

@anushkrishnav I approved the CL but you need to fix the CI errors by running black, cargo clippy. I also recommend running nox -edocs to confirm that the documentation compiles too

Will finish it today, thanks it was fun to work with !!

coveralls · 2024-06-10T14:20:58Z

Pull Request Test Coverage Report for Build 9449570809

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
58 unchanged lines in 7 files lost coverage.
Overall coverage decreased (-0.2%) to 95.87%

Files with Coverage Reduction	New Missed Lines	%
src/generators.rs	1	99.04%
src/coloring.rs	1	98.9%
rustworkx-core/src/generators/mod.rs	1	0.0%
rustworkx-core/src/dag_algo.rs	1	99.52%
rustworkx-core/src/generators/random_graph.rs	4	85.04%
src/traversal/mod.rs	6	96.98%
rustworkx-core/src/coloring.rs	44	90.02%

Totals
Change from base Build 9259965164:	-0.2%
Covered Lines:	17387
Relevant Lines:	18136

💛 - Coveralls

IvanIsCoding · 2024-06-10T14:48:15Z

the issue seems to stems from
'['git', 'describe', '--abbrev=0']'
Which ig gets the recent tag ?

That is an unrelated error. I think the real one is highlighted in the CI:

Error: <stdin>: syntax error in line 2 near '\'

Extension error:
Cell raised uncaught exception:
---------------------------------------------------------------------------
CalledProcessError                        Traceback (most recent call last)
Cell In[3], line 4
      1 import rustworkx as rx
      2 from rustworkx.visualization import graphviz_draw
----> 4 graphviz_draw(
      5 rx.generators.path_graph(2),
      6 filename="graph.svg",
      7 image_type="svg",
      8 node_attr_fn=lambdax:{"label":"the\nlabel","tooltip":"the\ntooltip"},
      9 )

File ~/work/rustworkx/rustworkx/.nox/docs-3/lib/python3.8/site-packages/rustworkx/visualization/graphviz.py:212, in graphviz_draw(graph, node_attr_fn, edge_attr_fn, graph_attr, filename, image_type, method)
    210     return image
    211 else:
--> 212     subprocess.run(
    213 [prog,"-T",output_format,"-o",filename],
    214 input=dot_str,
    215 check=True,
    216 encoding="utf8",
    217 text=True,
    218 )
    219     return None

The release note drawing is throwing an exception when calling graphviz

anushkrishnav · 2024-06-10T15:08:24Z

Okay I see the problem is with the quotes , it helps with the '' issue, once i changed it back its back to normal, I will updated the expected to make it in line with using quotes. Can you try again ? @IvanIsCoding

src/dot_utils.rs

coveralls · 2024-06-11T17:10:36Z

Pull Request Test Coverage Report for Build 9466635439

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
64 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.2%) to 95.837%

Files with Coverage Reduction	New Missed Lines	%
src/generators.rs	1	99.04%
src/coloring.rs	1	98.9%
rustworkx-core/src/generators/mod.rs	1	0.0%
rustworkx-core/src/dag_algo.rs	1	99.52%
rustworkx-core/src/generators/random_graph.rs	4	85.04%
src/traversal/mod.rs	6	96.98%
src/shortest_path/all_pairs_bellman_ford.rs	6	95.53%
rustworkx-core/src/coloring.rs	44	90.02%

Totals
Change from base Build 9259965164:	-0.2%
Covered Lines:	17379
Relevant Lines:	18134

💛 - Coveralls

IvanIsCoding

Leaving some minor change requests as unwrap() can panic the Rust code which is not very nice to handle from Python. If we return a result, though, it is much simpler

releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml

src/dot_utils.rs

releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml

…yaml

releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml

…yaml

coveralls · 2024-06-11T22:42:29Z

Pull Request Test Coverage Report for Build 9473598112

Details

7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.04%) to 95.832%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dot_utils.rs	7	8	87.5%

Files with Coverage Reduction	New Missed Lines	%
src/shortest_path/all_pairs_bellman_ford.rs	6	95.53%

Totals
Change from base Build 9450395122:	-0.04%
Covered Lines:	17382
Relevant Lines:	18138

💛 - Coveralls

releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml

…yaml

coveralls · 2024-06-11T23:00:40Z

Pull Request Test Coverage Report for Build 9473770325

Details

7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.04%) to 95.832%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dot_utils.rs	7	8	87.5%

Files with Coverage Reduction	New Missed Lines	%
src/shortest_path/all_pairs_bellman_ford.rs	6	95.53%

Totals
Change from base Build 9450395122:	-0.04%
Covered Lines:	17382
Relevant Lines:	18138

💛 - Coveralls

anushkrishnav · 2024-06-12T07:41:30Z

Awesome, thank you @IvanIsCoding I was fun working on this, Will work on more issue , I am learning rust for a the past 2 months and been exploring the code base to put my learning into practice thanks for the oppurtunity

This commit fixes a regression introduced in Qiskit#1203. That PR was attempting to fix missing character escaping in some string fields but it was doing so too eagerly. This would result in invalid dot files being produced for users upgrading from rustworkx < 0.15.0 that were wrapping strings in quotes as needed previously. For example if you were setting `'color="#aaaaaa"'` previously before this would become `color="\"#aaaaaa\""` after Qiskit#1203. In order to quickly release a 0.15.1 this commit reverts the dot generation component of Qiskit#1203 but updates the code to wrap tooltip in addition to label which was what the original bug reported. In 0.16.0 we should investigate adding a flag to control the escaping behavior of the function to either decide to wrap values in quotes or not.

This commit fixes a regression introduced in #1203. That PR was attempting to fix missing character escaping in some string fields but it was doing so too eagerly. This would result in invalid dot files being produced for users upgrading from rustworkx < 0.15.0 that were wrapping strings in quotes as needed previously. For example if you were setting `'color="#aaaaaa"'` previously before this would become `color="\"#aaaaaa\""` after #1203. In order to quickly release a 0.15.1 this commit reverts the dot generation component of #1203 but updates the code to wrap tooltip in addition to label which was what the original bug reported. In 0.16.0 we should investigate adding a flag to control the escaping behavior of the function to either decide to wrap values in quotes or not.

Potential solution

b5c395e

mtreinish reviewed May 31, 2024

View reviewed changes

Push update

e1c93e1

anushkrishnav requested a review from mtreinish May 31, 2024 10:29

anushkrishnav added 3 commits June 1, 2024 12:16

Fixed linting issues

e6fdc4c

Fix to linting issues

a33216f

minor fix

0e91fcd

IvanIsCoding reviewed Jun 1, 2024

View reviewed changes

src/dot_utils.rs Outdated Show resolved Hide resolved

using serde_json instead of manually escaping

62c5330

anushkrishnav requested a review from IvanIsCoding June 1, 2024 16:06

fix import, and added test case

1198483

IvanIsCoding reviewed Jun 4, 2024

View reviewed changes

Updated test logic

779cf41

anushkrishnav requested a review from IvanIsCoding June 4, 2024 11:56

IvanIsCoding reviewed Jun 4, 2024

View reviewed changes

tests/visualization/test_graphviz.py Outdated Show resolved Hide resolved

tests/visualization/test_graphviz.py Outdated Show resolved Hide resolved

updated test to check to_dot

08db533

anushkrishnav added 3 commits June 6, 2024 19:28

fix clippy issues

e34992d

created release notes

eada72e

fix release notes

5d80799

anushkrishnav requested a review from IvanIsCoding June 6, 2024 14:27

IvanIsCoding approved these changes Jun 7, 2024

View reviewed changes

fix linting issue in test

c4b08b7

anushkrishnav requested a review from IvanIsCoding June 8, 2024 04:51

anushkrishnav added 2 commits June 10, 2024 20:49

fixing "\" issue and updating tests

90b147b

updating more test to have quotes

15dd143

IvanIsCoding reviewed Jun 10, 2024

View reviewed changes

src/dot_utils.rs Outdated Show resolved Hide resolved

Update dot_utils.rs

ea682d7

IvanIsCoding approved these changes Jun 11, 2024

View reviewed changes

releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml Outdated Show resolved Hide resolved

src/dot_utils.rs Outdated Show resolved Hide resolved

src/dot_utils.rs Outdated Show resolved Hide resolved

IvanIsCoding added 4 commits June 11, 2024 17:40

Apply suggestions from code review

98549c8

Collect vector of results in Rust

fc3d0e6

Fix error handling

d69397f

Merge branch 'main' into anush_unitary

f75b894

IvanIsCoding approved these changes Jun 11, 2024

View reviewed changes

releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml Outdated Show resolved Hide resolved

Update releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.…

ddbe304

…yaml

IvanIsCoding added the automerge Queue a approved PR for merging label Jun 11, 2024

IvanIsCoding reviewed Jun 11, 2024

View reviewed changes

releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml Outdated Show resolved Hide resolved

Update releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.…

84f800e

…yaml

IvanIsCoding reviewed Jun 11, 2024

View reviewed changes

releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml Outdated Show resolved Hide resolved

Update releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.…

01cd323

…yaml

IvanIsCoding merged commit 70b7126 into Qiskit:main Jun 11, 2024
28 of 30 checks passed

IvanIsCoding mentioned this pull request Jun 19, 2024

Miscellaneous test improvements #1219

Merged

This was referenced Jun 28, 2024

Fix dot file generation #1229

Merged

Expose extra options to control string escaping in dot file generation #1230

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Potential fix to the tooltip with special characters bug #1203

Potential fix to the tooltip with special characters bug #1203

anushkrishnav commented May 30, 2024 •

edited

Loading

CLAassistant commented May 30, 2024 •

edited

Loading

mtreinish left a comment

anushkrishnav commented May 31, 2024

anushkrishnav commented May 31, 2024

IvanIsCoding left a comment

anushkrishnav commented Jun 1, 2024

anushkrishnav commented Jun 1, 2024 •

edited

Loading

IvanIsCoding Jun 4, 2024

anushkrishnav Jun 4, 2024

anushkrishnav Jun 4, 2024 •

edited

Loading

IvanIsCoding Jun 4, 2024

anushkrishnav Jun 4, 2024

IvanIsCoding commented Jun 6, 2024

IvanIsCoding left a comment

anushkrishnav commented Jun 8, 2024

coveralls commented Jun 10, 2024 •

edited

Loading

IvanIsCoding commented Jun 10, 2024

anushkrishnav commented Jun 10, 2024 •

edited

Loading

coveralls commented Jun 11, 2024 •

edited

Loading

IvanIsCoding left a comment

coveralls commented Jun 11, 2024 •

edited

Loading

coveralls commented Jun 11, 2024 •

edited

Loading

anushkrishnav commented Jun 12, 2024

Potential fix to the tooltip with special characters bug #1203

Potential fix to the tooltip with special characters bug #1203

Conversation

anushkrishnav commented May 30, 2024 • edited Loading

CLAassistant commented May 30, 2024 • edited Loading

mtreinish left a comment

Choose a reason for hiding this comment

anushkrishnav commented May 31, 2024

anushkrishnav commented May 31, 2024

IvanIsCoding left a comment

Choose a reason for hiding this comment

anushkrishnav commented Jun 1, 2024

anushkrishnav commented Jun 1, 2024 • edited Loading

IvanIsCoding Jun 4, 2024

Choose a reason for hiding this comment

anushkrishnav Jun 4, 2024

Choose a reason for hiding this comment

anushkrishnav Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

IvanIsCoding Jun 4, 2024

Choose a reason for hiding this comment

anushkrishnav Jun 4, 2024

Choose a reason for hiding this comment

IvanIsCoding commented Jun 6, 2024

IvanIsCoding left a comment

Choose a reason for hiding this comment

anushkrishnav commented Jun 8, 2024

coveralls commented Jun 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9449570809

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

IvanIsCoding commented Jun 10, 2024

anushkrishnav commented Jun 10, 2024 • edited Loading

coveralls commented Jun 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9466635439

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

IvanIsCoding left a comment

Choose a reason for hiding this comment

coveralls commented Jun 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9473598112

Details

💛 - Coveralls

coveralls commented Jun 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9473770325

Details

💛 - Coveralls

anushkrishnav commented Jun 12, 2024

anushkrishnav commented May 30, 2024 •

edited

Loading

CLAassistant commented May 30, 2024 •

edited

Loading

anushkrishnav commented Jun 1, 2024 •

edited

Loading

anushkrishnav Jun 4, 2024 •

edited

Loading

coveralls commented Jun 10, 2024 •

edited

Loading

anushkrishnav commented Jun 10, 2024 •

edited

Loading

coveralls commented Jun 11, 2024 •

edited

Loading

coveralls commented Jun 11, 2024 •

edited

Loading

coveralls commented Jun 11, 2024 •

edited

Loading