make override invocations more backwards-compatible

[dhellmann]

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:

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.

[dhellmann]

@tiran @Gregory-Pereira FYI, I've asked @shubhbapna to start looking at this.

[shubhbapna]

I have a draft PR open #197. I am trying to get all invocations of override functions using the new invoke function now but the functions in finders.py seem to have a different format than others: https://github.com/python-wheel-build/fromager/blob/main/src/fromager/finders.py#L24 https://github.com/python-wheel-build/fromager/blob/main/src/fromager/finders.py#L112

I am thinking I will shift the else part of these functions to another function and make them the default. But for find_source_dir it checks for 2 overrides before going into else . Any suggestions? I could make the default function as optional in invoke and if there is no override when default is optional it will raise an error

[shubhbapna]

Also in download_source we seem to be setting the value of source_type depending on whether the default was called or the override: https://github.com/python-wheel-build/fromager/blob/main/src/fromager/sources.py#L39

I could get the invoke function to return the value of the called function as well as a boolean to indicate whether default was called or not?

[shubhbapna]

Another option is to just pass the callable and named args only to invoke. So just like we have been doing, use find_override_method to get the override and then use that function directly in invoke. This way we keep the old code structure in most places and reduce the risk of breaking things.

downloader = overrides.find_override_method(req.name, "download_source")
source_type = "override"
if not downloader:
        downloader = default_download_source
        source_type = "sdist"
download_details = overrides.invoke(downloader, default_download_source, ctx=ctx, req=req, url=url)

[dhellmann]

The most important overrides are the ones for downloading and building things, since those are the most complex. Maybe focus on those for now and leave the finder invocations alone? Then if we run into an issue, one of the approaches you mention may become more clearly the best option and we'll know what to do.

[dhellmann]

Another option is to just pass the callable and named args only to invoke. So just like we have been doing, use find_override_method to get the override and then use that function directly in invoke. This way we keep the old code structure in most places and reduce the risk of breaking things.

downloader = overrides.find_override_method(req.name, "download_source")
source_type = "override"
if not downloader:
        downloader = default_download_source
        source_type = "sdist"
download_details = overrides.invoke(downloader, default_download_source, ctx=ctx, req=req, url=url)

I was hoping to reduce duplication, but this could be the way we do it for the finder functions, at least. Maybe we need a find_and_invoke() that's built using the existing find and the new invoke() that I described.

[shubhbapna]

The most important overrides are the ones for downloading and building things, since those are the most complex. Maybe focus on those for now and leave the finder invocations alone? Then if we run into an issue, one of the approaches you mention may become more clearly the best option and we'll know what to do.

Wouldn't maintaining both implementations be more difficult? especially when we start writing the downstream plugins more freely without worrying about function signatures?

[shubhbapna]

Another option is to just pass the callable and named args only to invoke. So just like we have been doing, use find_override_method to get the override and then use that function directly in invoke. This way we keep the old code structure in most places and reduce the risk of breaking things.

downloader = overrides.find_override_method(req.name, "download_source")
source_type = "override"
if not downloader:
        downloader = default_download_source
        source_type = "sdist"
download_details = overrides.invoke(downloader, default_download_source, ctx=ctx, req=req, url=url)

I was hoping to reduce duplication, but this could be the way we do it for the finder functions, at least. Maybe we need a find_and_invoke() that's built using the existing find and the new invoke() that I described.

I could do that and use find_and_invoke wherever possible. Get best of both worlds

[dhellmann]

The most important overrides are the ones for downloading and building things, since those are the most complex. Maybe focus on those for now and leave the finder invocations alone? Then if we run into an issue, one of the approaches you mention may become more clearly the best option and we'll know what to do.

Wouldn't maintaining both implementations be more difficult? especially when we start writing the downstream plugins more freely without worrying about function signatures?

I'm OK with doing it in stages. Not changing something will leave a bit of maintenance for the future, but that's OK.

Besides, if we land that download and rename feature you're working on, maybe we can get ride of the finder overrides?

[shubhbapna]

Another problem:

Now that we can use invoke, to allow using whatever args the plugins want won't we have to change the function signature of all the functions that use overrides? For example:

Changing download_source to:

def download_source(def download_source(
    ctx: context.WorkContext,
    req: Requirement,
    sdist_server_urls: list[str],
   **kwargs
)

Is this fine?

Edit: Now that I think about it we don't need this