Add option to save graph as PNG #523
Conversation
for more information, see https://pre-commit.ci
WalkthroughThis pull request focuses on removing the Changes
Sequence DiagramsequenceDiagram
participant User
participant Executor
participant ExecutorWithDependencies
participant PlotModule
User->>Executor: Create with plot_dependency_graph_filename
Executor->>ExecutorWithDependencies: Pass filename
ExecutorWithDependencies-->>PlotModule: Draw and save graph
PlotModule-->>User: Graph saved to specified file
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
executorlib/standalone/plot.py (1)
110-118: Improve docstring for the filename parameterThe parameter description should include that it's optional and specify supported file formats.
- filename (str): Name of the file to store the plotted graph in. + filename (Optional[str], optional): Path to save the graph visualization. + Supported formats: png, svg, pdf. Defaults to None for display output.executorlib/interactive/executor.py (2)
45-45: Fix inconsistent documentation for_generate_dependency_graphattribute.The attribute description is duplicated with conflicting types (bool vs str). This could lead to confusion.
Remove line 53 and update line 45 to accurately reflect the attribute's purpose:
- _generate_dependency_graph (bool): Whether to generate the dependency graph. - _generate_dependency_graph (str): Name of the file to store the plotted graph in. + _generate_dependency_graph (bool): Whether to generate and save the dependency graph.Also applies to: 53-53
81-85: Consider simplifying the graph generation logic.The current implementation can be made more concise while maintaining the same behavior.
- self._plot_dependency_graph_filename = plot_dependency_graph_filename - if plot_dependency_graph_filename is None: - self._generate_dependency_graph = plot_dependency_graph - else: - self._generate_dependency_graph = True + self._plot_dependency_graph_filename = plot_dependency_graph_filename + self._generate_dependency_graph = bool(plot_dependency_graph_filename) or plot_dependency_graph
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.ci_support/environment-mpich.yml(0 hunks).ci_support/environment-old.yml(0 hunks).ci_support/environment-openmpi.yml(0 hunks).ci_support/environment-win.yml(0 hunks)executorlib/__init__.py(5 hunks)executorlib/interactive/executor.py(4 hunks)executorlib/standalone/plot.py(2 hunks)pyproject.toml(1 hunks)tests/test_dependencies_executor.py(2 hunks)
💤 Files with no reviewable changes (4)
- .ci_support/environment-openmpi.yml
- .ci_support/environment-old.yml
- .ci_support/environment-mpich.yml
- .ci_support/environment-win.yml
🔇 Additional comments (3)
pyproject.toml (2)
42-43: LGTM: Good choice using networkx for graph visualization
The addition of networkx is appropriate for graph visualization capabilities, and the version is properly pinned.
44-48: LGTM: Well-structured dependency group
Good separation of notebook-specific dependencies into a new group. This allows users to install only what they need.
executorlib/standalone/plot.py (1)
1-4: LGTM: Clean import organization
Imports are well-organized and the Optional type is correctly imported for the new parameter.
| if filename is not None: | ||
| file_format = os.path.splitext(filename)[-1][1:] | ||
| with open(filename, "wb") as f: | ||
| f.write(nx.nx_agraph.to_agraph(graph).draw(prog="dot", format=file_format)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations
The file operations should include error handling for common issues like permission errors or invalid paths.
if filename is not None:
file_format = os.path.splitext(filename)[-1][1:]
+ if not file_format:
+ raise ValueError("Filename must have an extension (e.g., .png, .svg, .pdf)")
+ if file_format not in ['png', 'svg', 'pdf']:
+ raise ValueError(f"Unsupported file format: {file_format}")
+ try:
with open(filename, "wb") as f:
f.write(nx.nx_agraph.to_agraph(graph).draw(prog="dot", format=file_format))
+ except (OSError, IOError) as e:
+ raise IOError(f"Failed to save graph to {filename}: {str(e)}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if filename is not None: | |
| file_format = os.path.splitext(filename)[-1][1:] | |
| with open(filename, "wb") as f: | |
| f.write(nx.nx_agraph.to_agraph(graph).draw(prog="dot", format=file_format)) | |
| if filename is not None: | |
| file_format = os.path.splitext(filename)[-1][1:] | |
| if not file_format: | |
| raise ValueError("Filename must have an extension (e.g., .png, .svg, .pdf)") | |
| if file_format not in ['png', 'svg', 'pdf']: | |
| raise ValueError(f"Unsupported file format: {file_format}") | |
| try: | |
| with open(filename, "wb") as f: | |
| f.write(nx.nx_agraph.to_agraph(graph).draw(prog="dot", format=file_format)) | |
| except (OSError, IOError) as e: | |
| raise IOError(f"Failed to save graph to {filename}: {str(e)}") |
tests/test_dependencies_executor.py
Outdated
| @unittest.skipIf( | ||
| skip_graphviz_test, | ||
| "graphviz is not installed, so the plot_dependency_graph tests are skipped.", | ||
| ) | ||
| def test_executor_dependency_plot_filename(self): | ||
| graph_file = os.path.join(os.path.dirname(__file__), "test.png") | ||
| with Executor( | ||
| max_cores=1, | ||
| backend="local", | ||
| plot_dependency_graph=False, | ||
| plot_dependency_graph_filename=graph_file, | ||
| ) as exe: | ||
| cloudpickle_register(ind=1) | ||
| future_1 = exe.submit(add_function, 1, parameter_2=2) | ||
| future_2 = exe.submit(add_function, 1, parameter_2=future_1) | ||
| self.assertTrue(future_1.done()) | ||
| self.assertTrue(future_2.done()) | ||
| self.assertTrue(os.path.exists(graph_file)) | ||
| # os.remove(graph_file) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for graph file output
The test case needs improvements:
- The commented cleanup code should be uncommented
- Add tests for other file formats
- Add negative test cases for invalid formats/paths
def test_executor_dependency_plot_filename(self):
- graph_file = os.path.join(os.path.dirname(__file__), "test.png")
- with Executor(
- max_cores=1,
- backend="local",
- plot_dependency_graph=False,
- plot_dependency_graph_filename=graph_file,
- ) as exe:
- cloudpickle_register(ind=1)
- future_1 = exe.submit(add_function, 1, parameter_2=2)
- future_2 = exe.submit(add_function, 1, parameter_2=future_1)
- self.assertTrue(future_1.done())
- self.assertTrue(future_2.done())
- self.assertTrue(os.path.exists(graph_file))
- # os.remove(graph_file)
+ test_files = {
+ 'png': os.path.join(os.path.dirname(__file__), "test.png"),
+ 'svg': os.path.join(os.path.dirname(__file__), "test.svg"),
+ 'pdf': os.path.join(os.path.dirname(__file__), "test.pdf")
+ }
+
+ try:
+ # Test valid formats
+ for fmt, graph_file in test_files.items():
+ with Executor(
+ max_cores=1,
+ backend="local",
+ plot_dependency_graph=False,
+ plot_dependency_graph_filename=graph_file,
+ ) as exe:
+ cloudpickle_register(ind=1)
+ future_1 = exe.submit(add_function, 1, parameter_2=2)
+ future_2 = exe.submit(add_function, 1, parameter_2=future_1)
+ self.assertTrue(future_1.done())
+ self.assertTrue(future_2.done())
+ self.assertTrue(os.path.exists(graph_file))
+
+ # Test invalid format
+ with self.assertRaises(ValueError):
+ with Executor(
+ max_cores=1,
+ backend="local",
+ plot_dependency_graph_filename="test.invalid"
+ ) as exe:
+ future_1 = exe.submit(add_function, 1, parameter_2=2)
+
+ # Test invalid path
+ with self.assertRaises(IOError):
+ with Executor(
+ max_cores=1,
+ backend="local",
+ plot_dependency_graph_filename="/invalid/path/test.png"
+ ) as exe:
+ future_1 = exe.submit(add_function, 1, parameter_2=2)
+
+ finally:
+ # Cleanup
+ for graph_file in test_files.values():
+ if os.path.exists(graph_file):
+ os.remove(graph_file)Committable suggestion skipped: line range outside the PR's diff.
| return draw( | ||
| node_lst=node_lst, | ||
| edge_lst=edge_lst, | ||
| filename=self._plot_dependency_graph_filename, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations.
The draw function call should handle potential file operation errors when saving the graph.
- return draw(
- node_lst=node_lst,
- edge_lst=edge_lst,
- filename=self._plot_dependency_graph_filename,
- )
+ try:
+ return draw(
+ node_lst=node_lst,
+ edge_lst=edge_lst,
+ filename=self._plot_dependency_graph_filename,
+ )
+ except IOError as e:
+ raise IOError(f"Failed to save dependency graph: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return draw( | |
| node_lst=node_lst, | |
| edge_lst=edge_lst, | |
| filename=self._plot_dependency_graph_filename, | |
| ) | |
| try: | |
| return draw( | |
| node_lst=node_lst, | |
| edge_lst=edge_lst, | |
| filename=self._plot_dependency_graph_filename, | |
| ) | |
| except IOError as e: | |
| raise IOError(f"Failed to save dependency graph: {e}") from e |
| @@ -216,6 +220,7 @@ def __new__( | |||
| init_function=init_function, | |||
| refresh_rate=refresh_rate, | |||
| plot_dependency_graph=plot_dependency_graph, | |||
| plot_dependency_graph_filename=plot_dependency_graph_filename, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the filename parameter.
The plot_dependency_graph_filename parameter should be validated before being passed to _ExecutorWithDependencies.
Add a validation check similar to other parameters:
def _check_plot_dependency_graph_filename(filename: Optional[str]) -> None:
if filename is not None and not isinstance(filename, str):
raise TypeError("plot_dependency_graph_filename must be a string or None")Then use it before the _ExecutorWithDependencies instantiation:
+ _check_plot_dependency_graph_filename(plot_dependency_graph_filename)
return _ExecutorWithDependencies(
max_workers=max_workers,
...
Summary by CodeRabbit
New Features
Bug Fixes
Chores
matplotlibdependency from various environment configuration files and optional dependencies section.