Repository: python-wheel-build/fromager

Issue:
make override invocations more backwards-compatible
We recently added new arguments to some of the override function definitions and that is making it more difficult to upgrade existing uses in the repo downstream. It would be good if those new arguments were considered optional, since they're obviously not needed by existing implementations.

We could add a function to the `overrides` module to invoke an override, passing only the arguments that the function actually accepts.  It could be called from within fromager like this:

```python
download_details = overrides.invoke(req.name, "download_source", default_download_source,
    ctx=ctx, req=req, url=url)
```

The first 3 arguments are the name of the package, the name of the override, and the default function to call if there is no override for that package. The remaining named arguments are all args to "download_source".

`invoke()` should use `inspect` to look at the function signature of the thing being called and pass any named arguments that it accepts. For each value received by `invoke` that the override doesn't take should produce a warning log message like "{req.name}: {override_name} override {callable} does not take argument {arg_name}".

All invocations of override functions in fromager should be updated to use the new `invoke()` function.

Discussion:

Patch:
diff --git a/src/fromager/dependencies.py b/src/fromager/dependencies.py
index af7c578..073e28c 100644
--- a/src/fromager/dependencies.py
+++ b/src/fromager/dependencies.py
@@ -32,10 +32,15 @@ def get_build_system_dependencies(
         )
         return _read_requirements_file(build_system_req_file)
 
-    dep_func = overrides.find_override_method(req.name, "get_build_system_dependencies")
-    if not dep_func:
-        dep_func = default_get_build_system_dependencies
-    deps = _filter_requirements(req, dep_func(ctx, req, sdist_root_dir))
+    orig_deps = overrides.find_and_invoke(
+        req.name,
+        "get_build_system_dependencies",
+        default_get_build_system_dependencies,
+        ctx=ctx,
+        req=req,
+        sdist_root_dir=sdist_root_dir,
+    )
+    deps = _filter_requirements(req, orig_deps)
 
     _write_requirements_file(
         deps,
@@ -84,12 +89,15 @@ def get_build_backend_dependencies(
         )
         return _read_requirements_file(build_backend_req_file)
 
-    dep_func = overrides.find_override_method(
-        req.name, "get_build_backend_dependencies"
+    orig_deps = overrides.find_and_invoke(
+        req.name,
+        "get_build_backend_dependencies",
+        default_get_build_backend_dependencies,
+        ctx=ctx,
+        req=req,
+        sdist_root_dir=sdist_root_dir,
     )
-    if not dep_func:
-        dep_func = default_get_build_backend_dependencies
-    deps = _filter_requirements(req, dep_func(ctx, req, sdist_root_dir))
+    deps = _filter_requirements(req, orig_deps)
 
     _write_requirements_file(
         deps,
@@ -127,10 +135,15 @@ def get_build_sdist_dependencies(
         )
         return _read_requirements_file(build_sdist_req_file)
 
-    dep_func = overrides.find_override_method(req.name, "get_build_sdist_dependencies")
-    if not dep_func:
-        dep_func = default_get_build_sdist_dependencies
-    deps = _filter_requirements(req, dep_func(ctx, req, sdist_root_dir))
+    orig_deps = overrides.find_and_invoke(
+        req.name,
+        "get_build_sdist_dependencies",
+        default_get_build_sdist_dependencies,
+        ctx=ctx,
+        req=req,
+        sdist_root_dir=sdist_root_dir,
+    )
+    deps = _filter_requirements(req, orig_deps)
 
     _write_requirements_file(
         deps,
diff --git a/src/fromager/finders.py b/src/fromager/finders.py
index 18ef82f..fe50aa5 100644
--- a/src/fromager/finders.py
+++ b/src/fromager/finders.py
@@ -32,7 +32,10 @@ def find_sdist(
 
     if sdist_name_func:
         # The file must exist exactly as given.
-        sdist_file = downloads_dir / sdist_name_func(req, dist_version)
+        sdist_file_name = overrides.invoke(
+            sdist_name_func, req=req, dist_version=dist_version
+        )
+        sdist_file = downloads_dir / sdist_file_name
         if sdist_file.exists():
             return sdist_file
 
@@ -119,7 +122,10 @@ def find_source_dir(
     )
     if sdir_name_func:
         # The directory must exist exactly as given, inside the work_dir.
-        source_dir = work_dir / sdir_name_func(req, dist_version)
+        source_dir_name = overrides.invoke(
+            sdir_name_func, req=req, dist_version=dist_version
+        )
+        source_dir = work_dir / source_dir_name
         if source_dir.exists():
             return source_dir
         raise ValueError(f"looked for {source_dir} and did not find")
@@ -129,7 +135,9 @@ def find_source_dir(
     )
     if sdist_name_func:
         # The directory must exist exactly as given.
-        sdist_name = sdist_name_func(req, dist_version)
+        sdist_name = overrides.invoke(
+            sdist_name_func, req=req, dist_version=dist_version
+        )
         if sdist_name.endswith(".tar.gz"):
             ext_to_strip = ".tar.gz"
         elif sdist_name.endswith(".zip"):
diff --git a/src/fromager/overrides.py b/src/fromager/overrides.py
index 2e5aad7..ce646a5 100644
--- a/src/fromager/overrides.py
+++ b/src/fromager/overrides.py
@@ -1,3 +1,4 @@
+import inspect
 import itertools
 import logging
 import os
@@ -40,6 +41,24 @@ def _die_on_plugin_load_failure(
     raise RuntimeError(f"failed to load overrides for {ep.name}") from err
 
 
+def find_and_invoke(distname: str, method: str, default_fn: typing.Callable, **kwargs):
+    fn = find_override_method(distname, method)
+    if not fn:
+        fn = default_fn
+    return invoke(fn, **kwargs)
+
+
+def invoke(fn: typing.Callable, **kwargs):
+    sig = inspect.signature(fn)
+    for arg_name in list(kwargs):
+        if arg_name not in sig.parameters:
+            logger.warning(
+                f"{fn.__module__}.{fn.__name__} override does not take argument {arg_name}"
+            )
+            kwargs.pop(arg_name)
+    return fn(**kwargs)
+
+
 def log_overrides():
     logger.debug("loaded overrides for %s", _get_extensions().entry_points_names())
 
diff --git a/src/fromager/sources.py b/src/fromager/sources.py
index e19d152..2f5d75a 100644
--- a/src/fromager/sources.py
+++ b/src/fromager/sources.py
@@ -42,7 +42,12 @@ def download_source(
             logger.debug(
                 f"{req.name}: trying to resolve and download {req} using {url}"
             )
-            download_details = downloader(ctx, req, url)
+            download_details = overrides.invoke(
+                downloader,
+                ctx=ctx,
+                req=req,
+                sdist_server_url=url,
+            )
             if len(download_details) == 3:
                 source_filename, version, source_url = download_details
             elif len(download_details) == 2:
@@ -78,16 +83,17 @@ def resolve_dist(
     )
 
     # Create the (reusable) resolver. Limit to sdists.
-    provider_factory = overrides.find_override_method(req.name, "get_resolver_provider")
-    if not provider_factory:
-        provider_factory = default_resolver_provider
-    provider = provider_factory(
+    provider = overrides.find_and_invoke(
+        req.name,
+        "get_resolver_provider",
+        default_resolver_provider,
         ctx=ctx,
         req=req,
         include_sdists=include_sdists,
         include_wheels=include_wheels,
         sdist_server_url=sdist_server_url,
     )
+
     reporter = resolvelib.BaseReporter()
     rslvr = resolvelib.Resolver(provider, reporter)
 
@@ -301,10 +307,15 @@ def prepare_source(
     version: Version,
 ) -> pathlib.Path:
     logger.info(f"{req.name}: preparing source for {req} from {source_filename}")
-    preparer = overrides.find_override_method(req.name, "prepare_source")
-    if not preparer:
-        preparer = _default_prepare_source
-    source_root_dir = preparer(ctx, req, source_filename, version)
+    source_root_dir = overrides.find_and_invoke(
+        req.name,
+        "prepare_source",
+        _default_prepare_source,
+        ctx=ctx,
+        req=req,
+        source_filename=source_filename,
+        version=version,
+    )
     write_build_meta(source_root_dir.parent, req, source_filename, version)
     if source_root_dir is not None:
         logger.info(f"{req.name}: prepared source for {req} at {source_root_dir}")
@@ -332,11 +343,11 @@ def build_sdist(
     build_env: wheels.BuildEnvironment,
 ) -> pathlib.Path:
     logger.info(f"{req.name}: building source distribution in {sdist_root_dir}")
-    builder = overrides.find_override_method(req.name, "build_sdist")
-    if not builder:
-        builder = default_build_sdist
     extra_environ = overrides.extra_environ_for_pkg(ctx.envs_dir, req.name, ctx.variant)
-    sdist_filename = builder(
+    sdist_filename = overrides.find_and_invoke(
+        req.name,
+        "build_sdist",
+        default_build_sdist,
         ctx=ctx,
         extra_environ=extra_environ,
         req=req,
diff --git a/src/fromager/wheels.py b/src/fromager/wheels.py
index 1ed8875..6c5276d 100644
--- a/src/fromager/wheels.py
+++ b/src/fromager/wheels.py
@@ -80,9 +80,6 @@ def build_wheel(
     logger.info(
         f"{req.name}: building wheel for {req} in {sdist_root_dir} writing to {ctx.wheels_build}"
     )
-    builder = overrides.find_override_method(req.name, "build_wheel")
-    if not builder:
-        builder = default_build_wheel
     extra_environ = overrides.extra_environ_for_pkg(ctx.envs_dir, req.name, ctx.variant)
     # TODO: refactor?
     # Build Rust without network access
@@ -97,7 +94,10 @@ def build_wheel(
 
     # Start the timer
     start = datetime.now().replace(microsecond=0)
-    builder(
+    overrides.find_and_invoke(
+        req.name,
+        "build_wheel",
+        default_build_wheel,
         ctx=ctx,
         build_env=build_env,
         extra_environ=extra_environ,


Test patch:
diff --git a/tests/test_overrides.py b/tests/test_overrides.py
index 7030cd3..6bd7a9c 100644
--- a/tests/test_overrides.py
+++ b/tests/test_overrides.py
@@ -1,6 +1,8 @@
 import os
 import pathlib
+import typing
 from unittest import mock
+from unittest.mock import patch
 
 import pytest
 
@@ -147,3 +149,39 @@ def test_list_all(tmp_path: pathlib.Path):
     )
 
     assert expected == packages
+
+
+def test_invoke_override_with_exact_args():
+    def foo(arg1, arg2):
+        return arg1 is not None and arg2 is not None
+
+    assert overrides.invoke(foo, arg1="value1", arg2="value2")
+
+
+def test_invoke_override_with_more_args_than_needed():
+    def foo(arg1, arg2):
+        return arg1 is not None and arg2 is not None
+
+    assert overrides.invoke(foo, arg1="value1", arg2="value2", arg3="value3")
+
+
+def test_invoke_override_with_not_enough_args():
+    def foo(arg1, arg2):
+        return arg1 is not None and arg2 is not None
+
+    with pytest.raises(TypeError):
+        overrides.invoke(foo, arg1="value1")
+
+
+@patch("fromager.overrides.find_override_method")
+def test_find_and_invoke(
+    find_override_method: typing.Callable,
+):
+    def default_foo(arg1):
+        return arg1 is not None
+
+    find_override_method.return_value = None
+
+    assert overrides.find_and_invoke(
+        "pkg", "foo", default_foo, arg1="value1", arg2="value2"
+    )