Skip to content

Conversation

@sdelliot
Copy link
Collaborator

Refactoring to adjust how cycles are detected and output to users.

Description

Discovered as part of sandialabs/firewheel_repo_base#3, this PR refactors where cycles in the dependency graph are checked and assists in clearly printing this information for users, should a cycle appear.

Related Issue

N/A

Type of Change

Please select the type of change your pull request introduces:

  • Bugfix
  • New feature
  • Documentation update
  • Other (please describe): Refactor

Checklist

  • This PR conforms to the process detailed in the Contributing Guide.
  • I have included no proprietary/sensitive information in my code.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have tested my code.

Additional Notes

N/A

@github-actions github-actions bot added the refactor A code change that neither fixes a bug nor adds a feature label Nov 24, 2025
@sdelliot sdelliot requested a review from mitchnegus November 24, 2025 22:07
@sdelliot sdelliot changed the title Refactor: Refactoring to adjust how cycles are detected and output to users. refactor: Refactoring to adjust how cycles are detected and output to users. Nov 24, 2025
@sdelliot sdelliot marked this pull request as draft November 25, 2025 12:15
@sdelliot sdelliot marked this pull request as ready for review November 25, 2025 16:34
@sdelliot sdelliot requested a review from gregjacobus December 15, 2025 19:31
Comment on lines +166 to +202
def dependency_cycle_handler(self):
"""
The dependency graph had cycles so we should retrieve those cycles and alert
the user.

Raises:
UnsatisfiableDependenciesError: Output the cycles in the graph.
"""
all_human_cycles = self.get_cycles()
all_cycle_graphs = ""
for cycle in all_human_cycles:
cdg = nx.DiGraph()
for node in cycle:
cdg.add_node(node)

for i, node in enumerate(cycle[:-1]):
cdg.add_edge(node, cycle[i + 1])
cdg.add_edge(cycle[0], cycle[-1])

for line in nx.generate_network_text(cdg):
all_cycle_graphs += f"{line}\n"

all_cycle_graphs += "\n\n"

# Improving upon the default networkx diagrams
backedge: str = "╾"
all_cycle_graphs = all_cycle_graphs.replace(backedge, "◄─")
all_cycle_graphs = all_cycle_graphs.replace("╼", "►")

self.log.error(
"Unsatisfiable dependency graph contained %s cycles", len(all_human_cycles)
)
raise UnsatisfiableDependenciesError(
"Unsatisfiable: Circular dependency relationship(s) found.\n"
f"Simple cycles:\n{all_cycle_graphs}"
)

Copy link
Member

@mitchnegus mitchnegus Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're refactoring this, could we add a few more methods to make it a bit clearer and compartmentalize some of the implementation details?

Suggested change
def dependency_cycle_handler(self):
"""
The dependency graph had cycles so we should retrieve those cycles and alert
the user.
Raises:
UnsatisfiableDependenciesError: Output the cycles in the graph.
"""
all_human_cycles = self.get_cycles()
all_cycle_graphs = ""
for cycle in all_human_cycles:
cdg = nx.DiGraph()
for node in cycle:
cdg.add_node(node)
for i, node in enumerate(cycle[:-1]):
cdg.add_edge(node, cycle[i + 1])
cdg.add_edge(cycle[0], cycle[-1])
for line in nx.generate_network_text(cdg):
all_cycle_graphs += f"{line}\n"
all_cycle_graphs += "\n\n"
# Improving upon the default networkx diagrams
backedge: str = "╾"
all_cycle_graphs = all_cycle_graphs.replace(backedge, "◄─")
all_cycle_graphs = all_cycle_graphs.replace("╼", "►")
self.log.error(
"Unsatisfiable dependency graph contained %s cycles", len(all_human_cycles)
)
raise UnsatisfiableDependenciesError(
"Unsatisfiable: Circular dependency relationship(s) found.\n"
f"Simple cycles:\n{all_cycle_graphs}"
)
def dependency_cycle_handler(self):
"""
The dependency graph had cycles so we should retrieve those cycles and alert
the user.
Raises:
UnsatisfiableDependenciesError: Output the cycles in the graph.
"""
cycle_graphs = [self._build_cycle_graph(cycle) for cycle in self.get_cycles()]
cycle_graph_outputs = [
self._format_cycle_graph_output(cycle_graph)
for cycle_graph in cycle_graphs
]
all_cycle_graphs = "\n\n".join(cycle_graph_outputs)
self.log.error(
"Unsatisfiable dependency graph contained %s cycles", len(cycle_graphs)
)
raise UnsatisfiableDependenciesError(
"Unsatisfiable: Circular dependency relationship(s) found.\n"
f"Simple cycles:\n{all_cycle_graphs}"
)
def _build_cycle_graph(self, cycle):
cycle_graph = nx.DiGraph()
# Add nodes and edges to the cycle graph
for node in cycle:
cycle_graph.add_node(node)
for i, node in enumerate(cycle[:-1]):
next_node = cycle[i + 1]
cycle_graph.add_edge(node, next_node])
cycle_graph.add_edge(cycle[0], cycle[-1])
return cycle_graph
def _format_cycle_graph_output(self, cycle_graph)
raw_output_string = "\n".join(nx.generate_network_text(cycle_graph))
# Improve upon the default networkx diagrams
backedge: str = "╾"
output_string = raw_output_string.replace(backedge, "◄─").replace("╼", "►")
return output_string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor A code change that neither fixes a bug nor adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants