From 06eadb609a9048f699f4d4a1f7f8288cba90f64a Mon Sep 17 00:00:00 2001 From: Lance barto Date: Fri, 31 Oct 2025 13:54:51 -0400 Subject: [PATCH 1/5] feat: constraints.py adds support for markers in constraints Signed-off-by: Lance barto --- src/fromager/constraints.py | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/src/fromager/constraints.py b/src/fromager/constraints.py index f81a9f6e..11de73cc 100644 --- a/src/fromager/constraints.py +++ b/src/fromager/constraints.py @@ -15,7 +15,7 @@ class Constraints: def __init__(self) -> None: # mapping of canonical names to requirements # NOTE: Requirement.name is not normalized - self._data: dict[NormalizedName, Requirement] = {} + self._data: dict[NormalizedName, list[Requirement]] = {} def __iter__(self) -> Generator[NormalizedName, None, None]: yield from self._data @@ -24,14 +24,22 @@ def add_constraint(self, unparsed: str) -> None: """Add new constraint, must not conflict with any existing constraints""" req = Requirement(unparsed) canon_name = canonicalize_name(req.name) - previous = self._data.get(canon_name) - if previous is not None: - raise KeyError( - f"{canon_name}: new constraint '{req}' conflicts with '{previous}'" - ) - if requirements_file.evaluate_marker(req, req): - logger.debug(f"adding constraint {req}") - self._data[canon_name] = req + marker_key = str(req.marker) if req.marker else "" + previous = self._data.get(canon_name, []) + + # Check for conflicts with existing constraints + for existing_req in previous: + existing_marker_key = str(existing_req.marker) if existing_req.marker else "" + + # If markers match (including both being empty), it's a conflict + if marker_key == existing_marker_key: + raise KeyError( + f"{canon_name}: new constraint '{req}' conflicts with existing constraint '{existing_req}'" + ) + + if canon_name not in self._data: + self._data[canon_name] = [] + self._data[canon_name].append(req) def load_constraints_file(self, constraints_file: str | pathlib.Path) -> None: """Load constraints from a constraints file""" @@ -41,7 +49,14 @@ def load_constraints_file(self, constraints_file: str | pathlib.Path) -> None: self.add_constraint(line) def get_constraint(self, name: str) -> Requirement | None: - return self._data.get(canonicalize_name(name)) + # Lookup the list by the key given (name), iterate through the list + # call evaluate_marker(req, req) until it returns true, then return that + constraints = self._data.get(canonicalize_name(name), []) + + for constraint in constraints: + if requirements_file.evaluate_marker(constraint, constraint): + return constraint + return None def allow_prerelease(self, pkg_name: str) -> bool: constraint = self.get_constraint(pkg_name) From ed9f2a572161dac93be36baaa90bef59a93b0a8b Mon Sep 17 00:00:00 2001 From: Lance barto Date: Fri, 31 Oct 2025 13:55:32 -0400 Subject: [PATCH 2/5] test: adds marker tests for test_constraints.py Signed-off-by: Lance barto --- tests/test_constraints.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/test_constraints.py b/tests/test_constraints.py index 6e0c96e0..d05de4e7 100644 --- a/tests/test_constraints.py +++ b/tests/test_constraints.py @@ -37,15 +37,36 @@ def test_add_constraint_conflict(): c = constraints.Constraints() c.add_constraint("foo<=1.1") c.add_constraint("flit_core==2.0rc3") + + # Exact duplicate should raise error (same package, same marker) with pytest.raises(KeyError): c.add_constraint("foo<=1.1") + + # Different version, same marker (no marker) should raise error with pytest.raises(KeyError): c.add_constraint("foo>1.1") + + # Different version for flit_core should raise error with pytest.raises(KeyError): c.add_constraint("flit_core>2.0.0") + + # Normalized name conflict should raise error with pytest.raises(KeyError): c.add_constraint("flit-core>2.0.0") + # Same package with different markers should NOT raise error + c.add_constraint("foo==1.0; platform_machine != 'ppc64le'") + c.add_constraint("foo==1.1; platform_machine == 'ppc64le'") + + # But same package with same marker should raise error + with pytest.raises(KeyError): + c.add_constraint("foo==1.2; platform_machine != 'ppc64le'") + + # Verify multiple constraints for same package are stored + assert len(c._data[constraints.canonicalize_name("foo")]) == 3 + assert len(c._data[constraints.canonicalize_name("flit_core")]) == 1 + assert len(c._data) == 2 + def test_allow_prerelease(): c = constraints.Constraints() From fc7f1c4192fe8b5e18a1a4b5204632fcfc391b40 Mon Sep 17 00:00:00 2001 From: Lance barto Date: Fri, 31 Oct 2025 14:01:49 -0400 Subject: [PATCH 3/5] test: adds tests for get_constraint() Signed-off-by: Lance barto --- tests/test_constraints.py | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/test_constraints.py b/tests/test_constraints.py index d05de4e7..e8327e95 100644 --- a/tests/test_constraints.py +++ b/tests/test_constraints.py @@ -92,3 +92,48 @@ def test_load_constraints_file(tmp_path: pathlib.Path): c.load_constraints_file(constraint_file) assert list(c) == ["egg", "torch"] # type: ignore assert c.get_constraint("torch") == Requirement("torch==3.1.0") + +def test_get_constraint(): + c = constraints.Constraints() + + # Test: No constraint returns None + assert c.get_constraint("nonexistent") is None + + # Test: Single constraint without marker + c.add_constraint("foo==1.0") + constraint = c.get_constraint("foo") + assert constraint is not None + assert str(constraint.specifier) == "==1.0" + assert constraint.marker is None + + # Test: Multiple constraints with markers - should return the one matching current platform + c.add_constraint("bar==1.0; platform_machine == 'x86_64'") + c.add_constraint("bar==2.0; platform_machine == 'ppc64le'") + c.add_constraint("bar==3.0; platform_machine == 'arm64'") + + # get_constraint should return whichever one matches the current platform + constraint = c.get_constraint("bar") + assert constraint is not None + assert str(constraint.specifier) in ["==1.0", "==2.0", "==3.0"] + + # Test: Constraint with marker that evaluates to False returns None + c.add_constraint("baz==1.0; python_version < '2.0'") + # This marker should never match (we're not on Python < 2.0) + constraint = c.get_constraint("baz") + assert constraint is None + + # Test: Constraint with marker that evaluates to True + c.add_constraint("qux==1.0; python_version >= '3.0'") + constraint = c.get_constraint("qux") + assert constraint is not None + assert str(constraint.specifier) == "==1.0" + + # Test: Name normalization (foo-bar vs foo_bar) + c.add_constraint("my-package==1.0") + constraint = c.get_constraint("my_package") + assert constraint is not None + assert str(constraint.specifier) == "==1.0" + + constraint = c.get_constraint("my-package") + assert constraint is not None + assert str(constraint.specifier) == "==1.0" From a344d1d81fe2a56bd7b43ccc351b5c1462f74435 Mon Sep 17 00:00:00 2001 From: Lance barto Date: Fri, 31 Oct 2025 14:39:18 -0400 Subject: [PATCH 4/5] chore: runs linter Signed-off-by: Lance barto --- src/fromager/constraints.py | 4 +++- tests/test_constraints.py | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/fromager/constraints.py b/src/fromager/constraints.py index 11de73cc..d40576c7 100644 --- a/src/fromager/constraints.py +++ b/src/fromager/constraints.py @@ -29,7 +29,9 @@ def add_constraint(self, unparsed: str) -> None: # Check for conflicts with existing constraints for existing_req in previous: - existing_marker_key = str(existing_req.marker) if existing_req.marker else "" + existing_marker_key = ( + str(existing_req.marker) if existing_req.marker else "" + ) # If markers match (including both being empty), it's a conflict if marker_key == existing_marker_key: diff --git a/tests/test_constraints.py b/tests/test_constraints.py index e8327e95..20bee8b5 100644 --- a/tests/test_constraints.py +++ b/tests/test_constraints.py @@ -93,6 +93,7 @@ def test_load_constraints_file(tmp_path: pathlib.Path): assert list(c) == ["egg", "torch"] # type: ignore assert c.get_constraint("torch") == Requirement("torch==3.1.0") + def test_get_constraint(): c = constraints.Constraints() From ca0ee6cb55a1811a7cc8cded23fad5eac55cc3e7 Mon Sep 17 00:00:00 2001 From: Lance barto Date: Fri, 31 Oct 2025 15:36:49 -0400 Subject: [PATCH 5/5] fix: updates constraints.py to only add relevant constraints to the environment This should allow for duplicate constraints, as long as they don't conflict --- src/fromager/constraints.py | 38 ++++++---------- tests/test_constraints.py | 89 ++++++++++++++++--------------------- 2 files changed, 51 insertions(+), 76 deletions(-) diff --git a/src/fromager/constraints.py b/src/fromager/constraints.py index d40576c7..f41ac459 100644 --- a/src/fromager/constraints.py +++ b/src/fromager/constraints.py @@ -15,7 +15,7 @@ class Constraints: def __init__(self) -> None: # mapping of canonical names to requirements # NOTE: Requirement.name is not normalized - self._data: dict[NormalizedName, list[Requirement]] = {} + self._data: dict[NormalizedName, Requirement] = {} def __iter__(self) -> Generator[NormalizedName, None, None]: yield from self._data @@ -24,24 +24,19 @@ def add_constraint(self, unparsed: str) -> None: """Add new constraint, must not conflict with any existing constraints""" req = Requirement(unparsed) canon_name = canonicalize_name(req.name) - marker_key = str(req.marker) if req.marker else "" - previous = self._data.get(canon_name, []) + previous = self._data.get(canon_name) - # Check for conflicts with existing constraints - for existing_req in previous: - existing_marker_key = ( - str(existing_req.marker) if existing_req.marker else "" - ) - - # If markers match (including both being empty), it's a conflict - if marker_key == existing_marker_key: - raise KeyError( - f"{canon_name}: new constraint '{req}' conflicts with existing constraint '{existing_req}'" - ) + if not requirements_file.evaluate_marker(req, req): + logger.debug(f"Constraint {req} does not match environment") + return - if canon_name not in self._data: - self._data[canon_name] = [] - self._data[canon_name].append(req) + if previous is not None: + raise KeyError( + f"{canon_name}: new constraint '{req}' conflicts with '{previous}'" + ) + if requirements_file.evaluate_marker(req, req): + logger.debug(f"adding constraint {req}") + self._data[canon_name] = req def load_constraints_file(self, constraints_file: str | pathlib.Path) -> None: """Load constraints from a constraints file""" @@ -51,14 +46,7 @@ def load_constraints_file(self, constraints_file: str | pathlib.Path) -> None: self.add_constraint(line) def get_constraint(self, name: str) -> Requirement | None: - # Lookup the list by the key given (name), iterate through the list - # call evaluate_marker(req, req) until it returns true, then return that - constraints = self._data.get(canonicalize_name(name), []) - - for constraint in constraints: - if requirements_file.evaluate_marker(constraint, constraint): - return constraint - return None + return self._data.get(canonicalize_name(name)) def allow_prerelease(self, pkg_name: str) -> bool: constraint = self.get_constraint(pkg_name) diff --git a/tests/test_constraints.py b/tests/test_constraints.py index 20bee8b5..f9253800 100644 --- a/tests/test_constraints.py +++ b/tests/test_constraints.py @@ -1,6 +1,8 @@ import pathlib +import typing import pytest +from packaging import markers from packaging.requirements import Requirement from packaging.version import Version @@ -54,18 +56,49 @@ def test_add_constraint_conflict(): with pytest.raises(KeyError): c.add_constraint("flit-core>2.0.0") + # Different, but equivalent markers should raise KeyError + with pytest.raises(KeyError): + c.add_constraint( + "bar==1.0; python_version >= '3.11' and platform_machine == 'x86_64'" + ) + c.add_constraint( + "bar==1.1; platform_machine == 'x86_64' and python_version >= '3.11'" + ) + c.add_constraint( + "bar==1.0; python_version >= '3.11' and platform_machine == 'arm64'" + ) + c.add_constraint( + "bar==1.1; platform_machine == 'arm64' and python_version >= '3.11'" + ) + # Same package with different markers should NOT raise error - c.add_constraint("foo==1.0; platform_machine != 'ppc64le'") - c.add_constraint("foo==1.1; platform_machine == 'ppc64le'") + c.add_constraint("baz==1.0; platform_machine != 'ppc64le'") + c.add_constraint("baz==1.1; platform_machine == 'ppc64le'") # But same package with same marker should raise error with pytest.raises(KeyError): c.add_constraint("foo==1.2; platform_machine != 'ppc64le'") # Verify multiple constraints for same package are stored - assert len(c._data[constraints.canonicalize_name("foo")]) == 3 - assert len(c._data[constraints.canonicalize_name("flit_core")]) == 1 - assert len(c._data) == 2 + assert len(c._data) == 4 # flit_core, foo, bar, and baz + + # Make sure correct constraint is added + env = typing.cast(dict[str, str], markers.default_environment()) + constraint = c.get_constraint("bar") + + if env.get("platform_machine") == "x86_64" and constraint is not None: + assert constraint.name == "bar" + assert constraint.specifier == "==1.0" + assert constraint.marker == markers.Marker( + 'python_version >= "3.11" and platform_machine == "x86_64"' + ) + + if env.get("platform_machine") == "arm64" and constraint is not None: + assert constraint.name == "bar" + assert constraint.specifier == "==1.0" + assert constraint.marker == markers.Marker( + 'python_version >= "3.11" and platform_machine == "arm64"' + ) def test_allow_prerelease(): @@ -92,49 +125,3 @@ def test_load_constraints_file(tmp_path: pathlib.Path): c.load_constraints_file(constraint_file) assert list(c) == ["egg", "torch"] # type: ignore assert c.get_constraint("torch") == Requirement("torch==3.1.0") - - -def test_get_constraint(): - c = constraints.Constraints() - - # Test: No constraint returns None - assert c.get_constraint("nonexistent") is None - - # Test: Single constraint without marker - c.add_constraint("foo==1.0") - constraint = c.get_constraint("foo") - assert constraint is not None - assert str(constraint.specifier) == "==1.0" - assert constraint.marker is None - - # Test: Multiple constraints with markers - should return the one matching current platform - c.add_constraint("bar==1.0; platform_machine == 'x86_64'") - c.add_constraint("bar==2.0; platform_machine == 'ppc64le'") - c.add_constraint("bar==3.0; platform_machine == 'arm64'") - - # get_constraint should return whichever one matches the current platform - constraint = c.get_constraint("bar") - assert constraint is not None - assert str(constraint.specifier) in ["==1.0", "==2.0", "==3.0"] - - # Test: Constraint with marker that evaluates to False returns None - c.add_constraint("baz==1.0; python_version < '2.0'") - # This marker should never match (we're not on Python < 2.0) - constraint = c.get_constraint("baz") - assert constraint is None - - # Test: Constraint with marker that evaluates to True - c.add_constraint("qux==1.0; python_version >= '3.0'") - constraint = c.get_constraint("qux") - assert constraint is not None - assert str(constraint.specifier) == "==1.0" - - # Test: Name normalization (foo-bar vs foo_bar) - c.add_constraint("my-package==1.0") - constraint = c.get_constraint("my_package") - assert constraint is not None - assert str(constraint.specifier) == "==1.0" - - constraint = c.get_constraint("my-package") - assert constraint is not None - assert str(constraint.specifier) == "==1.0"