Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix cycles in when traversing graph #520

Merged
merged 1 commit into from
Dec 6, 2024
Merged

fix cycles in when traversing graph #520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/fromager/dependency_graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ def get_dependency_edges(
visited = set()
for edge in self._depth_first_traversal(
self.nodes[ROOT].children,
set(),
match_dep_types=match_dep_types,
):
if edge.destination_node.key not in visited:
Expand Down Expand Up @@ -278,13 +279,17 @@ def get_install_dependency_versions(

def _depth_first_traversal(
self,
start_node: list[DependencyEdge],
start_edges: list[DependencyEdge],
visited: set[str],
match_dep_types: list[RequirementType] | None = None,
) -> typing.Iterable[DependencyEdge]:
for edge in start_node:
for edge in start_edges:
if edge.destination_node.key in visited:
continue
if match_dep_types and edge.req_type not in match_dep_types:
continue
visited.add(edge.destination_node.key)
Copy link
Member

Choose a reason for hiding this comment

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

We should add the start nodes, too, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

they will get added when they are being process in the loop

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the start nodes from self.nodes[ROOT].children on line 243 will be added unless they also appear as a destination node of something else, right?

One way to address it would be to add start_node to the visited set as the first thing this function does. That way every node that is traversed is added. I think if you make that change, you don't need line 291, because the set will be updated during the recursion on line 293.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I think the confusion is the name of the variable start_nodes. It should be start_edges because children is an array of edges. I have added a test case that adds a duplicate toplevel entry (start node) and the test passes

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK.

yield edge
yield from self._depth_first_traversal(
edge.destination_node.children, match_dep_types
edge.destination_node.children, visited, match_dep_types
)
38 changes: 38 additions & 0 deletions tests/test_graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,41 @@ def test_get_install_dependencies():
for node in graph.get_install_dependencies()
]
assert install_nodes == ["a==2.0", "d==6.0", "b==3.0", "e==6.0"]


def test_cycles_get_install_dependencies():
graph = dependency_graph.DependencyGraph.from_dict(raw_graph)
# create cycle: a depends on d and d depends on a
graph.add_dependency(
parent_name=canonicalize_name("a"),
parent_version=Version("2.0"),
req_type=requirements_file.RequirementType.INSTALL,
req=Requirement("d>=4.0"),
req_version=Version("6.0"),
download_url="url for d",
)

graph.add_dependency(
parent_name=canonicalize_name("d"),
parent_version=Version("6.0"),
req_type=requirements_file.RequirementType.INSTALL,
req=Requirement("a<=2.0"),
req_version=Version("2.0"),
download_url="url for a",
)

# add another duplicate toplevel
graph.add_dependency(
parent_name=None,
parent_version=None,
req_type=requirements_file.RequirementType.TOP_LEVEL,
req=Requirement("a<=2.0"),
req_version=Version("2.0"),
download_url="url for a",
)

install_nodes = [
f"{node.to_dict()['canonicalized_name']}=={node.to_dict()['version']}"
for node in graph.get_install_dependencies()
]
assert install_nodes == ["a==2.0", "d==6.0"]
Loading