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

GitHub Action for binary wheels releases #71

Merged
merged 3 commits into from
Jul 25, 2023
Merged

GitHub Action for binary wheels releases #71

merged 3 commits into from
Jul 25, 2023

Conversation

siddharthab
Copy link
Contributor

@siddharthab siddharthab commented Jul 21, 2023

This is not very well tested, but can be a beginning.

The release workflow is currently only on-demand and not automatic. We will also need to remove the "dev" tag, so it can automatically pick up the right release tags. We can also set it based on the torchsort version instead of any git tag.

I chose a version matrix based on what I wanted to test in my environments, but it can be easily changed.

You can see how the built artifacts look at https://github.com/siddharthab/torchsort/releases/tag/dev

Fixes #70.

@teddykoker
Copy link
Owner

teddykoker commented Jul 21, 2023

Thanks for this. It looks like there are several linux specific things in the action (e.g. disk space cleanup and cuda installation), but I think it would be easy enough to modify in the future for other OS support if there is demand. My only concern is that the torch version is not included in the wheel name, so it looks like the runs for different torch versions are overriding each other.

I think it would make sense to keep the workflow on demand at the moment. I'm not sure what would make the most sense to pull as the release tag. Using the current git tag makes sense, but would then require a new tag if we, for example, needed to modify the build action and run it again (if I'm understanding this all correctly). For this reason it might make more sense to pull the torchsort version, and always use "v" + <torchsort version> as the release tag, in which case wheels could be added or re-built if necessary. The downside of this would be that wheel files could potentially be modified over time, which could cause reproducibility issues if the links to the wheels are being used in requirement.txt files for example; I'm not sure how much of an issue this could be though, and preventing the action from overriding existing files could prevent this as well.

Don't worry about the failing tests; merging from main will fix.

@siddharthab
Copy link
Contributor Author

Thank you so much for the quick response.

I agree that it will be easy to generalize it to more OS-es if and when needed, but we can start with this.

I added the torch version to the dist name. That was a good catch. Thank you.

For release tag and mutability, it should be easy to configure this to the way you want the releases. The GitHub Action we use is very flexible in that regard. I think the best practice here is that you would tag a git commit as a particular version and then trigger a release manually. I have modified this PR to assume that is what you would be doing. I have also made it such that the release once written can not be modified, so you can be assured that files will not change.

People also have workflows in which the release process would also release to PyPi automatically (and PyPi has detailed instructions on how to do that), so you can fold that in if you want.

@siddharthab
Copy link
Contributor Author

It might take one or two more attempts after we merge this PR to make it all work.

@teddykoker
Copy link
Owner

I think the best practice here is that you would tag a git commit as a particular version and then trigger a release manually. I have modified this PR to assume that is what you would be doing. I have also made it such that the release once written can not be modified, so you can be assured that files will not change.

I would agree that this seems like best practice. My only concern would be if we wanted to make a change to the action code itself without updating the version. For example, if we wanted to add new torch versions to the matrix, or add support for other OS. In this case I feel it wouldn't make sense to have to update the version of the library since there were no functional changes to the library itself. Do you know if it would be possible for the action to, for example, use the last published tag available? That way changes to the action would not necessitate a new version number.

Alternatively, I think it would be a good idea to run this on push/pull-request and then use the upload artifact functionality to preview the wheels. This would allow us to debug the actions without having to do too many merges or releases:

# perform on PRs, pushes, manual dispatch
on: [push,pull_request,workflow_dispatch]

...
steps:

... 
     # upload wheels as build artifacts
     # would ensure building works in PRs, or changes to build action
    - name: upload
      uses: actions/upload-artifact@v3
      with:
        name: wheels
        path: "*/*.whl"

     # release and add wheels to release
     # would only be run on new tag, which could be made after the artifacts above are verified,
     # minimizing number of releases needed 
    - name: release
      uses: softprops/action-gh-release@v1
      if: ${{startsWith(github.ref, 'refs/tags/') }}
      with:
          files: "*/*.whl"

People also have workflows in which the release process would also release to PyPi automatically (and PyPi has detailed instructions on how to do that), so you can fold that in if you want.

I have added this to other repos of mine in the past - I will likely add this functionality once this is sorted out, but I think it would remain separate as, for the reasons discussed above, we might not always want to publish a new version every time we want to build wheels.

@siddharthab
Copy link
Contributor Author

Do you know if it would be possible for the action to, for example, use the last published tag available?

I think we can always use the torchsort version as the release version instead of relying on a git tag, or take the release version as an input to the manual dispatch. And we can keep updating the files in the same release, including adding new types in the version matrix. However, overwriting the same files will lead to confusion.

Your alternative is also a good solution in which we use pull requests and just the uploaded artifacts to inspect and download the files for these iterations. Artifacts are retained only for some time so people can not rely on them.

My recommendation:

  1. For manual dispatch, take the release version from the user as an input, and allow releases to be updated.
  2. For push and pull requests, stop after uploading the artifacts and do not perform the release.

I will update this PR shortly to do these. I think this will address both your concerns.

@siddharthab
Copy link
Contributor Author

Now updated. Please take a look. I think this addresses all your concerns. You can also merge this now and experiment with the built artifacts through pull requests or the various release mechanisms afforded by https://github.com/ncipollo/release-action.

@teddykoker
Copy link
Owner

Thanks. Looks good for now, with the artifacts being generated here. Will merge and try on releases now.

@teddykoker teddykoker merged commit 523829c into teddykoker:main Jul 25, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Binary wheels
2 participants