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

Prebuilt release naming convention #73

Merged
merged 4 commits into from
Jul 26, 2023
Merged

Prebuilt release naming convention #73

merged 4 commits into from
Jul 26, 2023

Conversation

teddykoker
Copy link
Owner

@teddykoker teddykoker commented Jul 25, 2023

Align closer to the Wheel Naming Convention and torch's naming convention. For example the version built with torch 1.13, CUDA 11.7 and Python 3.10 would have the following name:

torchsort-0.1.9+pt113cu117-cp310-cp310-linux_x86_64.whl

@teddykoker
Copy link
Owner Author

@siddharthab just updated the naming convention to be a little more consistent. Let me know if you have any concerns, otherwise I think this is good for now, and I will run the release portion so the wheels are permanent.

@siddharthab
Copy link
Contributor

This looks good. I have relatively little experience with the Python ecosystem, so I just put in something reasonable, but your format looks cleaner.

@@ -57,6 +57,9 @@ jobs:
run: |
export FORCE_CUDA=1
export TORCH_CUDA_ARCH_LIST="3.5;5.0+PTX;6.0;7.0;7.5;8.0;8.6"
TORCH_VERSION=`echo "pt${{ matrix.torch-version }}" | sed "s/..$//" | sed "s/\.//g"`
Copy link
Contributor

@siddharthab siddharthab Jul 25, 2023

Choose a reason for hiding this comment

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

..$ is a little brittle because it will always remove the last two characters. If you want to remove the patch version and any suffixes, i.e. just get the major and minor version, then 's/^\([0-9]\+\.[0-9]\+\).*$/\1/' is an option.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks! Have fixed

@siddharthab
Copy link
Contributor

Also, maybe before you release, you may want to remove cuda 11.3 from the matrix. I was testing it for my own application and forgot to take it out before merging the PR but I don't think this project should support it.

@teddykoker
Copy link
Owner Author

Also, maybe before you release, you may want to remove cuda 11.3 from the matrix. I was testing it for my own application and forgot to take it out before merging the PR but I don't think this project should support it.

Done

@teddykoker teddykoker merged commit 6bf7968 into main Jul 26, 2023
17 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.

2 participants