python-wheel-build / fromager

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

Implement settings driven download #191

Closed shubhbapna closed 1 month ago

shubhbapna commented 1 month ago

Most of the downstream plugins have a common pattern - resolve package from a different source index, download package from a predefined URL and renaming downloaded tarball.

Predefined URL and renaming downloaded tarball was added but we need the full feature set working to remove it from the plugins:

shubhbapna commented 1 month ago

@dhellmann I am considering the following settings.yaml configuration as an option:

torch:
     download_source:
           url: "url"
           rename_to: "url"
     resolve_dist:
           sdist_server_url: "url"
           include_sdist: true
           include_wheels: false

The idea is to have all the necessary settings for a package in one place rather than what I had defined in the previous #164.

I think this settings represents the common case we have in all our downstream plugins

dhellmann commented 1 month ago

@dhellmann I am considering the following settings.yaml configuration as an option:

torch:
     download_source:
           url: "url"
           rename_to: "url"
     resolve_dist:
           sdist_server_url: "url"
           include_sdist: true
           include_wheels: false

The idea is to have all the necessary settings for a package in one place rather than what I had defined in the previous #164.

I think this settings represents the common case we have in all our downstream plugins

So far I've tried to make a distinction between settings that might change in different parts of the pipeline, and settings that are "fixed". For the values likely to change, a CLI option is easier than modifying the config file.

I think the sdist_server_url is something that would change, so that should stay a CLI option. It's also going to be 1 value for the run, not per-package.

The other items you have here are in the "fixed" category. It's not clear what "include" means for the flags, unless you know the code, so let's think about potential other names that might make more sense to a user.

Then to structure the data, I think we want to stick with a top level key with a name like "packages", then your "torch" key would be under that. That separates it from the pre_built lists, for example. Unless you think we can map the pre-built stuff into this structure, too?

shubhbapna commented 1 month ago

I think the sdist_server_url is something that would change, so that should stay a CLI option. It's also going to be 1 value for the run, not per-package.

But we are already seeing plugins overriding this value to use the public pypi index

shubhbapna commented 1 month ago

The other items you have here are in the "fixed" category. It's not clear what "include" means for the flags, unless you know the code, so let's think about potential other names that might make more sense to a user.

We could come up with a better name but these configuration changes are to help the users who write plugins. A user who writes a plugin already knows these names

shubhbapna commented 1 month ago

Then to structure the data, I think we want to stick with a top level key with a name like "packages", then your "torch" key would be under that. That separates it from the pre_built lists, for example. Unless you think we can map the pre-built stuff into this structure, too?

That make sense. I don't want to remap prebuilt since that will create backward compatibility issues

dhellmann commented 1 month ago

I think the sdist_server_url is something that would change, so that should stay a CLI option. It's also going to be 1 value for the run, not per-package.

But we are already seeing plugins overriding this value to use the public pypi index

All to the same alternative value, though, right?

dhellmann commented 1 month ago

The other items you have here are in the "fixed" category. It's not clear what "include" means for the flags, unless you know the code, so let's think about potential other names that might make more sense to a user.

We could come up with a better name but these configuration changes are to help the users who write plugins. A user who writes a plugin already knows these names

If the point is to not have to write the plugins, they may not need to get as far as learning the lookup code.

shubhbapna commented 1 month ago

I think the sdist_server_url is something that would change, so that should stay a CLI option. It's also going to be 1 value for the run, not per-package.

But we are already seeing plugins overriding this value to use the public pypi index

All to the same alternative value, though, right?

Yes. So a cli option for an alternate would be preferable rather than mentioning it in settings.yaml?