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

Implement settings driven download #191

Closed
3 tasks
shubhbapna opened this issue Jul 17, 2024 · 8 comments · Fixed by #207
Closed
3 tasks

Implement settings driven download #191

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

Comments

@shubhbapna
Copy link
Collaborator

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

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

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

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

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

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

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

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

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?

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