search

Qiskit / rustworkx

A high performance Python graph library implemented in Rust.
https://www.rustworkx.org
Apache License 2.0
966 stars 140 forks source link

Potential fix to the tooltip with special characters bug #1203

Closed anushkrishnav closed 3 weeks ago

anushkrishnav commented 1 month ago

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

CLAassistant commented 1 month ago

CLA assistant check
All committers have signed the CLA.

anushkrishnav commented 1 month ago

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 commented 1 month ago

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

image

it would be saved as a graph.svg ??

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

anushkrishnav commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

anushkrishnav commented 1 month ago

@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 !!

IvanIsCoding commented 1 month ago

@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 !!

You missed the ruff lint from nox -elint. Also seconding nox -edocs try to make sure the documentation builds

anushkrishnav commented 4 weeks ago

@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 !!

You missed the ruff lint from nox -elint. Also seconding nox -edocs try to make sure the documentation builds

I am having some issue with nox -edocs

nox > jupyter kernelspec list
Available kernels:
  python3    /Users/anushkrishnav/Library/Jupyter/kernels/python3
nox > cd docs
nox > sphinx-build -W -d build/.doctrees -b html source build/html
Running Sphinx v7.1.2

Extension error:
Handler <function _get_versions at 0x1048e1260> for event 'config-inited' threw an exception (exception: Command '['git', 'describe', '--abbrev=0']' returned non-zero exit status 128.)
nox > Command sphinx-build -W -d build/.doctrees -b html source build/html failed with exit code 2
nox > Session docs-3 failed.
IvanIsCoding commented 4 weeks ago

We are getting test failures like

======================================================================
FAIL: test_graph_to_dot (graph.test_dot.TestDot.test_graph_to_dot)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/rustworkx/rustworkx/tests/graph/test_dot.py", line 51, in test_graph_to_dot
    self.assertEqual(expected, res)
AssertionError: 'graph {\n0 [color=black, fillcolor=green, label="a", style=[96 chars]n}\n' != 'graph {\n0 [color="black", fillcolor="green", label="a", st[110 chars]n}\n'
  graph {
- 0 [color=black, fillcolor=green, label="a", style=filled];
+ 0 [color="black", fillcolor="green", label="a", style="filled"];
?          +     +            +     +                   +      +
- 1 [color=black, fillcolor=red, label="a", style=filled];
+ 1 [color="black", fillcolor="red", label="a", style="filled"];
?          +     +            +   +                   +      +
- 0 -- 1 [label="1", name=1];
+ 0 -- 1 [label="1", name="1"];
?                         + +
  }

Ideally we would not have any. So you either need to update the test to include the quotes or make the code not generate the quotes. Up to you

IvanIsCoding commented 3 weeks ago

@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 !!

You missed the ruff lint from nox -elint. Also seconding nox -edocs try to make sure the documentation builds

I am having some issue with nox -edocs

nox > jupyter kernelspec list
Available kernels:
  python3    /Users/anushkrishnav/Library/Jupyter/kernels/python3
nox > cd docs
nox > sphinx-build -W -d build/.doctrees -b html source build/html
Running Sphinx v7.1.2

Extension error:
Handler <function _get_versions at 0x1048e1260> for event 'config-inited' threw an exception (exception: Command '['git', 'describe', '--abbrev=0']' returned non-zero exit status 128.)
nox > Command sphinx-build -W -d build/.doctrees -b html source build/html failed with exit code 2
nox > Session docs-3 failed.

I am going to need a little bit more of the error message output to identify the error. But I have a feeling it is the space identation on the release note. Those are always a bit tricky

anushkrishnav commented 3 weeks ago

I am going to need a little bit more of the error message output to identify the error. But I have a feeling it is the space identation on the release note. Those are always a bit tricky

@IvanIsCoding This is the message I am getting from the terminal atm 


nox > Running session docs-3
nox > Re-using existing virtual environment at .nox/docs-3.
nox > python -m pip install setuptools-rust fixtures 'testtools>=2.5.0' 'networkx>=2.5' 'scipy>=1.7' 'stestr>=4.1'
nox > python -m pip install '.[all]' -c constraints.txt
nox > python -m pip install -r docs/source/requirements.txt -c constraints.txt
nox > python -m ipykernel install --user
Installed kernelspec python3 in /Users/anushkrishnav/Library/Jupyter/kernels/python3
nox > jupyter kernelspec list
Available kernels:
  python3    /Users/anushkrishnav/Library/Jupyter/kernels/python3
nox > cd docs
nox > sphinx-build -W -d build/.doctrees -b html source build/html
Running Sphinx v7.1.2

Extension error:
Handler <function _get_versions at 0x102505260> for event 'config-inited' threw an exception (exception: Command '['git', 'describe', '--abbrev=0']' returned non-zero exit status 128.)
nox > Command sphinx-build -W -d build/.doctrees -b html source build/html failed with exit code 2
nox > Session docs-3 failed.
anushkrishnav commented 3 weeks ago

the issue seems to stems from 

'['git', 'describe', '--abbrev=0']'

Which ig gets the recent tag ?

coveralls commented 3 weeks ago

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.

Details

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%
<!-- Total: 58 -->
Totals Coverage Status
Change from base Build 9259965164: -0.2%
Covered Lines: 17387
Relevant Lines: 18136
💛 - Coveralls
IvanIsCoding commented 3 weeks ago

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 commented 3 weeks ago

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

coveralls commented 3 weeks ago

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.

Details

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%
<!-- Total: 64 -->
Totals Coverage Status
Change from base Build 9259965164: -0.2%
Covered Lines: 17379
Relevant Lines: 18134
💛 - Coveralls
coveralls commented 3 weeks ago

Pull Request Test Coverage Report for Build 9473598112

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/dot_utils.rs 7 8 87.5%
<!-- Total: 7 8 87.5% -->
Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_bellman_ford.rs 6 95.53%
<!-- Total: 6 -->
Totals Coverage Status
Change from base Build 9450395122: -0.04%
Covered Lines: 17382
Relevant Lines: 18138
💛 - Coveralls
coveralls commented 3 weeks ago

Pull Request Test Coverage Report for Build 9473770325

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/dot_utils.rs 7 8 87.5%
<!-- Total: 7 8 87.5% -->
Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_bellman_ford.rs 6 95.53%
<!-- Total: 6 -->
Totals Coverage Status
Change from base Build 9450395122: -0.04%
Covered Lines: 17382
Relevant Lines: 18138
💛 - Coveralls
anushkrishnav commented 3 weeks ago

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