Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a much higher level override for the resolver #185

Closed
shubhbapna opened this issue Jul 15, 2024 · 6 comments · Fixed by #207
Closed

Provide a much higher level override for the resolver #185

shubhbapna opened this issue Jul 15, 2024 · 6 comments · Fixed by #207
Assignees
Labels
enhancement New feature or request

Comments

@shubhbapna
Copy link
Collaborator

shubhbapna commented Jul 15, 2024

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
Copy link
Collaborator Author

@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
Copy link
Member

@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 shubhbapna self-assigned this Jul 15, 2024
@shubhbapna
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants