From a925786909c3280e666710d31ad518600c9c3a5c Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Mon, 20 Jan 2025 16:21:24 -0500 Subject: [PATCH] fix some issues writing the constraints file during bootstrap We're seeing some cases downstream where fromager fails with an exception because it tries to remove something from the unresolved dependency list when the item isn't in the list. This seems to be caused by having multiple candidate versions for a package where both versions could be used by all consumers. This change resolves the issue by stopping the resolution loop when we find a version of a package that can work for all consumers. The loop now iterates from the newest version to the oldest, to ensure we choose the newest package that works. There is also some error handling in case this fix is not sufficient to eliminate the original ValueError. I also noticed that a log message that we supposed to print debug info about the candidate versions was using the wrong list variable so it was always printing the same values, regardless of the package. I cleaned that up and added additional debug logging. --- src/fromager/commands/bootstrap.py | 34 ++++++++++++----- tests/test_bootstrap.py | 59 ++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/src/fromager/commands/bootstrap.py b/src/fromager/commands/bootstrap.py index 4bdc9634..cb43178b 100644 --- a/src/fromager/commands/bootstrap.py +++ b/src/fromager/commands/bootstrap.py @@ -219,7 +219,7 @@ def write_constraints_file( # Make copy of the original list and loop over unresolved dependencies for dep_name, nodes in unresolved_dependencies[:]: # Track which versions can be used by which parent requirement. - usable_versions: dict[str, list[Version]] = {} + usable_versions: dict[Version, list[Version]] = {} # Track how many total users of a requirement (by name) there are so we # can tell later if any version can be used by all of them. user_counter = 0 @@ -245,7 +245,7 @@ def write_constraints_file( for matching_version in parent_edge.req.specifier.filter( dep_versions ): - usable_versions.setdefault(str(matching_version), []).append( + usable_versions.setdefault(matching_version, []).append( parent_edge.destination_node.version ) user_counter += 1 @@ -254,18 +254,34 @@ def write_constraints_file( # and output that if we find it. Otherwise, include a warning and report # all versions so a human reading the file can make their own decision # about how to resolve the conflict. - for v, users in usable_versions.items(): - if len(users) == user_counter: - version_strs = [str(v) for v in sorted(versions)] + for v, users in reversed(sorted(usable_versions.items())): + if len(users) != user_counter: logger.debug( - "%s: selecting %s from multiple candidates %s", + "%s: version %s is useable by %d of %d consumers, skipping it", dep_name, v, - version_strs, + len(users), + user_counter, ) - resolved[dep_name] = Version(v) - resolved_something = True + continue + version_strs = [str(v) for v in reversed(sorted(dep_versions))] + logger.debug( + "%s: selecting %s from multiple candidates %s", + dep_name, + v, + version_strs, + ) + resolved[dep_name] = v + resolved_something = True + try: unresolved_dependencies.remove((dep_name, nodes)) + except ValueError: + logger.debug( + "%s: %s not in unresolved dependencies list, ignoring", + dep_name, + (dep_name, nodes), + ) + break # Write resolved versions to constraints file for dep_name, resolved_version in sorted(resolved.items()): diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 5faf31fd..a245b8f6 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -261,3 +261,62 @@ def test_write_constraints_file_duplicates(): d==1.0 """).lstrip() assert expected == buffer.getvalue() + + +def test_write_constraints_file_multiples(): + buffer = io.StringIO() + raw_graph = { + "": { + "download_url": "", + "pre_built": False, + "version": "0", + "canonicalized_name": "", + "edges": [ + { + "key": "a==2.7.0", + "req_type": "toplevel", + "req": "a==2.7.0", + }, + { + "key": "b==0.26.1", + "req_type": "toplevel", + "req": "b==0.26.1", + }, + ], + }, + "b==0.26.1": { + "download_url": "", + "pre_built": False, + "version": "0.26.1", + "canonicalized_name": "b", + "edges": [], + }, + "b==0.26.2": { + "download_url": "", + "pre_built": False, + "version": "0.26.2", + "canonicalized_name": "b", + "edges": [], + }, + "a==2.7.0": { + "download_url": "", + "pre_built": False, + "version": "2.7.0", + "canonicalized_name": "a", + "edges": [ + { + "key": "b==0.26.2", + "req_type": "install", + "req": "b<0.27.0,>=0.26.1", + }, + ], + }, + } + graph = dependency_graph.DependencyGraph.from_dict(raw_graph) + assert bootstrap.write_constraints_file(graph, buffer) + expected = textwrap.dedent(""" + a==2.7.0 + # NOTE: fromager selected b==0.26.2 from: ['0.26.1', '0.26.2'] + b==0.26.2 + """).lstrip() + assert expected == buffer.getvalue()