python-wheel-build / fromager

Build your own wheels
https://pypi.org/project/fromager/
Apache License 2.0
3 stars 9 forks source link

Provide a much higher level override for the resolver #185

Closed shubhbapna closed 1 month ago

shubhbapna commented 1 month ago

Right now we can override an entire resolver to perform the same sort of resolution during source lookup by using a different source of truth for version info. We also need to provide a way to override resolution at a higher level, so that plugins can use the python package index resolver with a different server, so plugins can hard-code a version, or so plugins and resolve versions using pre-built wheels when sdists are not available.

shubhbapna commented 1 month ago

@dhellmann if we just need to provide a way to override where the resolver looks, why not just allow fromager to accept multiple sdists URL? It seems to be easier than writing a plugin to just override the sdist server URL?

dhellmann commented 1 month ago

@dhellmann if we just need to provide a way to override where the resolver looks, why not just allow fromager to accept multiple sdists URL? It seems to be easier than writing a plugin to just override the sdist server URL?

That's not all we need.

The pyarrow plugin needs to look for wheels upstream, not sdists, but the torch and vllm plugins need to essentially hard-code versions.

shubhbapna commented 1 month ago

Now that I think about do we really need a separate override for this? If the user wants to override the sdist server url then they can achieve that by defining get_resolver_provider in the following way:

def get_resolver_provider(
    ctx: context.WorkContext,
    req: Requirement,
    sdist_server_url: str,
    include_sdists: bool,
    include_wheels: bool,
) -> resolver.PyPIProvider:
    return default_resolver_provider(
                 ctx,
                 req,
                 sdist_server_url="whatever url"
                 include_sdists=False # whatever the user wants
                 include_wheels=True # whatever the user wants
    )

As for hardcoding the versions, it makes sense to override download_source entirely since you may or may not want to skip resolution depending on your use case like for triton we are skipping resolution while for vllm we are not

shubhbapna commented 1 month ago

If the goal again is to reduce the duplicate code in plugins and if we believe that this will be a common case, then we will need to add additional fields to settings.yaml

dhellmann commented 1 month ago

Now that I think about do we really need a separate override for this? If the user wants to override the sdist server url then they can achieve that by defining get_resolver_provider in the following way:

def get_resolver_provider(
    ctx: context.WorkContext,
    req: Requirement,
    sdist_server_url: str,
    include_sdists: bool,
    include_wheels: bool,
) -> resolver.PyPIProvider:
    return default_resolver_provider(
                 ctx,
                 req,
                 sdist_server_url="whatever url"
                 include_sdists=False # whatever the user wants
                 include_wheels=True # whatever the user wants
    )

As for hardcoding the versions, it makes sense to override download_source entirely since you may or may not want to skip resolution depending on your use case like for triton we are skipping resolution while for vllm we are not

This is a good point.

What if we ship with that function in fromager, maybe called get_pypi_resolver_provider(), and then plugins would just have to import it to use it?

shubhbapna commented 1 month ago

What if we ship with that function in fromager, maybe called get_pypi_resolver_provider(), and then plugins would just have to import it to use it?

They will still have to define get_resolver_provider and call get_pypi_resolver_provider in it. Moreover, since we want to allow customizing the sdist_server_url, include_sdists and include_wheels, the function signature for get_pypi_resolver_provider will almost be the same as default_resolver_provider. So the resolver plugin will look almost the same as the one defined here anyways