Merge pull request #314 from dhellmann/per-package-settings-directory

add package-specific settings files

docs/customization.md

@@ -455,3 +455,25 @@ packages:
             include_sdists: false
         build_dir: directory name relative to sdist directory, defaults to an empty string, which means to use the sdist directory
 ```
+
+### Customizations using package-specific settings files
+
+In addition to the unified settings file, package settings can be saved in their
+own file in the directory specified with `--settings-dir`. Files should be named
+using the canonicalized form of the package name with the suffix `.yaml`. The
+data from the file is treated as though it appears in the
+`packages.package_name` section of the main settings file.  Files are read in
+lexicographical order. Settings in package-specific files override all settings
+in the global file.
+
+```yaml
+# settings/torch.yaml
+download_source:
+    url: "https://github.com/pytorch/pytorch/releases/download/v${version}/pytorch-v${version}.tar.gz"
+    destination_filename: "${canonicalized_name}-${version}.tar.gz"
+resolver_dist:
+    sdist_server_url: "https://pypi.org/simple"
+    include_wheels: true
+    include_sdists: false
+build_dir: directory name relative to sdist directory, defaults to an empty string, which means to use the sdist directory
+```

src/fromager/__main__.py

@@ -90,6 +90,12 @@
     type=clickext.ClickPath(),
     help="location of the application settings file",
 )
+@click.option(
+    "--settings-dir",
+    default=pathlib.Path("overrides/settings"),
+    type=clickext.ClickPath(),
+    help="location of per-package settings files",
+)
 @click.option(
     "-c",
     "--constraints-file",
@@ -132,6 +138,7 @@ def main(
     patches_dir: pathlib.Path,
     envs_dir: pathlib.Path,
     settings_file: pathlib.Path,
+    settings_dir: pathlib.Path,
     constraints_file: pathlib.Path,
     wheel_server_url: str,
     cleanup: bool,
@@ -178,7 +185,7 @@ def main(
         ctx.fail(f"network isolation is not available: {NETWORK_ISOLATION_ERROR}")
 
     wkctx = context.WorkContext(
-        active_settings=settings.load(settings_file),
+        active_settings=settings.load(settings_file, settings_dir),
         pkg_constraints=constraints.load(constraints_file),
         patches_dir=patches_dir,
         envs_dir=envs_dir,

src/fromager/settings.py

@@ -23,10 +23,7 @@ def pre_built(self, variant: str) -> set[str]:
         return set(overrides.pkgname_to_override_module(n) for n in names)
 
     def packages(self) -> dict[str, dict[str, str]]:
-        p = self._return_value_or_default(self._data.get("packages"), {})
-        return {
-            overrides.pkgname_to_override_module(key): value for key, value in p.items()
-        }
+        return self._return_value_or_default(self._data.get("packages"), {})
 
     def download_source_url(
         self,
@@ -148,16 +145,47 @@ def _resolve_template(
         raise
 
 
-def _parse(content: str) -> Settings:
-    data = yaml.safe_load(content)
-    return Settings(data)
+def load(settings_file: pathlib.Path, settings_dir: pathlib.Path) -> Settings:
+    settings_data = {}
 
-
-def load(filename: pathlib.Path) -> Settings:
-    filepath = pathlib.Path(filename)
+    filepath = pathlib.Path(settings_file)
     if not filepath.exists():
         logger.debug("settings file %s does not exist, ignoring", filepath.absolute())
-        return Settings({})
-    with open(filepath, "r") as f:
-        logger.info("loading settings from %s", filepath.absolute())
-        return _parse(f.read())
+    else:
+        with open(filepath, "r") as f:
+            logger.info("loading settings from %s", filepath.absolute())
+            settings_data = yaml.safe_load(f.read())
+
+    # Per-package files are inserted in the `packages` key using the name that
+    # will be used to look the value up. Transform any existing keys to that
+    # format so we can warn if there are overriding values.
+    package_settings_from = {}
+    if "packages" not in settings_data:
+        settings_data["packages"] = {}
+    else:
+        new_packages = {}
+        for name, value in settings_data["packages"].items():
+            package_name = overrides.pkgname_to_override_module(name)
+            new_packages[package_name] = value
+            package_settings_from[package_name] = settings_file
+        settings_data["packages"] = new_packages
+
+    for package_file in sorted(settings_dir.glob("*.yaml")):
+        package_name = overrides.pkgname_to_override_module(package_file.stem)
+        with open(package_file, "r") as f:
+            logger.info(
+                "%s: loading settings from %s",
+                package_name,
+                package_file.absolute(),
+            )
+            pkg_data = yaml.safe_load(f.read())
+            if package_name in settings_data["packages"]:
+                logger.warn(
+                    "%s: discarding settings from %s",
+                    package_name,
+                    package_settings_from[package_name],
+                )
+            settings_data["packages"][package_name] = pkg_data
+            package_settings_from[package_name] = package_file
+
+    return Settings(settings_data)

tests/test_settings.py

@@ -2,19 +2,25 @@
 import textwrap
 
 import pytest
+import yaml
 from packaging.requirements import Requirement
 from packaging.version import Version
 
 from fromager import settings
 
 
+def _parse(content: str) -> settings.Settings:
+    data = yaml.safe_load(content)
+    return settings.Settings(data)
+
+
 def test_empty():
     s = settings.Settings({})
     assert s.pre_built("cuda") == set()
 
 
 def test_no_pre_built():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     pre_built:
     """)
@@ -23,7 +29,7 @@ def test_no_pre_built():
 
 
 def test_with_pre_built():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     pre_built:
       cuda:
@@ -35,7 +41,7 @@ def test_with_pre_built():
 
 
 def test_with_download_source():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
       foo:
@@ -51,7 +57,7 @@ def test_with_download_source():
 
 
 def test_no_download_source():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
     """)
@@ -61,7 +67,7 @@ def test_no_download_source():
 
 
 def test_with_resolver_dist():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
       foo:
@@ -79,7 +85,7 @@ def test_with_resolver_dist():
 
 
 def test_no_resolver_dist():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
       foo:
@@ -94,7 +100,7 @@ def test_no_resolver_dist():
 
 
 def test_relative_path_build_dir():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
       foo:
@@ -106,7 +112,7 @@ def test_relative_path_build_dir():
 
 
 def test_only_name_build_dir():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
       foo:
@@ -118,7 +124,7 @@ def test_only_name_build_dir():
 
 
 def test_absolute_path_build_dir():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
       foo:
@@ -130,7 +136,7 @@ def test_absolute_path_build_dir():
 
 
 def test_escape_sdist_root_build_dir():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
       foo:
@@ -143,7 +149,7 @@ def test_escape_sdist_root_build_dir():
 
 
 def test_changelog():
-    s = settings._parse(
+    s = _parse(
         textwrap.dedent("""
     packages:
       foo:
@@ -173,3 +179,64 @@ def test_resolve_template_with_no_matching_template():
     req = Requirement("foo==1.0")
     with pytest.raises(KeyError):
         settings._resolve_template("url-${flag}", req, "1.0")
+
+
+def test_load_package_files(tmp_path):
+    settings_filename = tmp_path / "settings.yaml"
+    settings_filename.write_text(
+        textwrap.dedent("""
+            packages:
+                foo:
+                    build_dir: "../tmp/build"
+        """)
+    )
+    other_settings = tmp_path / "settings"
+    other_settings.mkdir()
+    (other_settings / "bar.yaml").write_text(
+        textwrap.dedent("""
+        build_dir: "bar-build-dir"
+        """)
+    )
+    s = settings.load(settings_filename, other_settings)
+    assert s._data == {
+        "packages": {
+            "foo": {"build_dir": "../tmp/build"},
+            "bar": {"build_dir": "bar-build-dir"},
+        },
+    }
+
+
+def test_load_package_files_precedence(tmp_path):
+    # In this test we want to show both that settings from a package-specific
+    # file override the global values and that 2 files for the same package with
+    # different filename but same canonical form are handled in order. We can't
+    # guarantee that we are running on a case-sensitive filesystem (macOS), so
+    # use "." and "-" as differentiating forms and rely on their sort order. Use
+    # the non-canonical form of the name in the global file to show that it is
+    # also transformed into the canonical form.
+    settings_filename = tmp_path / "settings.yaml"
+    settings_filename.write_text(
+        textwrap.dedent("""
+            packages:
+                foo-bar:
+                    build_dir: "../tmp/build"
+        """)
+    )
+    other_settings = tmp_path / "settings"
+    other_settings.mkdir()
+    (other_settings / "foo-bar.yaml").write_text(
+        textwrap.dedent("""
+        build_dir: "dash-build-dir"
+        """)
+    )
+    (other_settings / "foo.bar.yaml").write_text(
+        textwrap.dedent("""
+        build_dir: "dotted-build-dir"
+        """)
+    )
+    s = settings.load(settings_filename, other_settings)
+    assert s._data == {
+        "packages": {
+            "foo_bar": {"build_dir": "dotted-build-dir"},
+        },
+    }