Skip to content

Conversation

@shubhbapna
Copy link
Contributor

@shubhbapna shubhbapna commented Jul 10, 2024

Fixes #138

User can provide a mapping of package to download source url in settings.yaml:

download_source:
  torch:
      url: "https://github.com/pytorch/pytorch/releases/download/v${version}/pytorch-v${version}.tar.gz"
      rename_to: "torch-${version}"

User can define a predefined url for a package from which all its sources will be downloaded from. Optionally they can rename the downloaded sdist to whatever they want.

The only supported template variables are: version. version is replaced by the version returned by the resolver.

@mergify mergify bot added the ci label Jul 10, 2024
@shubhbapna shubhbapna force-pushed the custom-tarball-url branch 2 times, most recently from 393008d to 87eb1ed Compare July 11, 2024 18:06
@shubhbapna shubhbapna marked this pull request as ready for review July 11, 2024 18:07
@shubhbapna shubhbapna requested a review from dhellmann July 11, 2024 18:13
Copy link
Member

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update https://github.com/python-wheel-build/fromager/blob/main/src/fromager/overrides.py#L131 as well, please, to include these new types of overrides.

custom_download_urls = ctx.settings.url_to_download_sdist_from(req.name)
rename_to_template = ctx.settings.rename_sdist_to(req.name)

# don't include sdists if the user wants to download source from a predefined url
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear why this change is related to having the URLs. Maybe because we're assuming there are no sdists for those cases? That happens to be true for some of the examples we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user is providing a url from which they want to download the sdist then the url that the resolver returns for the source dist won't be used anyways

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including or excluding specific formats influences the resolver, too. It will only return a version the distribution type we've asked for exists for that version.

I guess in most cases where we download source from an alternative location there isn't an sdist to use, so this is probably OK. We may end up wanting to expose options to control which types of distributions to consider, though. Let's deal with that in a separate PR. In fact, we may want to split the step for resolving the version from the step for downloading the source for that version.

It would be really good to see a functioning version of this PR with a real use case based on our downstream builds.

@shubhbapna
Copy link
Contributor Author

Could you update https://github.com/python-wheel-build/fromager/blob/main/src/fromager/overrides.py#L131 as well, please, to include these new types of overrides.

Sorry, I didn't quite understand what I should add here?

@shubhbapna shubhbapna requested a review from dhellmann July 11, 2024 20:49
@jwieleRH
Copy link

I was thinking of doing something like this myself for two reasons:

  1. triton downloads the source from github in a file named by the version. This would be a reusable way to get the source and give the source archive a sensible name.
  2. sacrebleu upstream builds its source distribution with setuptools, which leaves CHANGELOG.md out of the sdist which means you can't build the wheel from the sdist. Currently, we have a workaround that just creates an empty CHANGELOG.md file. Downloading the source from github instead of PyPI would allow us to build the wheel exactly the same way the upstream does, and of course the rename_to option would have the same application as with triton.

@shubhbapna shubhbapna requested a review from tiran July 12, 2024 17:43
@shubhbapna shubhbapna force-pushed the custom-tarball-url branch 2 times, most recently from 12290b5 to 4b0d4ce Compare July 12, 2024 17:58
@shubhbapna shubhbapna force-pushed the custom-tarball-url branch from 62ed16f to 4741cc6 Compare July 12, 2024 18:45
custom_download_urls = ctx.settings.url_to_download_sdist_from(req.name)
rename_to_template = ctx.settings.rename_sdist_to(req.name)

# don't include sdists if the user wants to download source from a predefined url
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including or excluding specific formats influences the resolver, too. It will only return a version the distribution type we've asked for exists for that version.

I guess in most cases where we download source from an alternative location there isn't an sdist to use, so this is probably OK. We may end up wanting to expose options to control which types of distributions to consider, though. Let's deal with that in a separate PR. In fact, we may want to split the step for resolving the version from the step for downloading the source for that version.

It would be really good to see a functioning version of this PR with a real use case based on our downstream builds.

@mergify mergify bot merged commit a0fcbe8 into python-wheel-build:main Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

provide builtins for downloading GitHub release tarballs

4 participants