-
Notifications
You must be signed in to change notification settings - Fork 30
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
Use git describe to get version and bump actions to newest versions too #79
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @PiotrCzapla for this PR.
Will this solve #78 character limit issue on windows as well ?
setup.py
Outdated
@@ -170,12 +170,20 @@ def repair_wheel(self): | |||
self.copy_file(repaired_wheel, wheel_path) | |||
print(f"Copied repaired wheel to: {wheel_path}") | |||
|
|||
def get_version_from_git(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is running a subprocess calling the git command a good decision here ?
There si no better way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not, sure... but calling git
is an easy and stable solution, but maybe not directly in setup.py as git
is apparently not installed on the build docker images. Torch is running git
too but they approach to version generation is more elaborate and probably runs before the build on the host machine. Although adding a version.py like torch, makes a lot of sense. We could put there the build configuration of whispercpp too, it will simplify debugging later. So let me see if I can run a script right after checkout.
Sure it wont. I meant it will give a workaround for windows users, they will be able to use pre complied wheels, especially once we provide versions with 'cuda' and 'vulkan' (Intel) support. |
24b2d48
to
3621dc6
Compare
It seems pip don't like '-' in the version. Here are few recent version from torch pip list:
They store git sha in the version.py, like so:
But that is for the pulished artifacts, where they set the versioning numbers through CI using environment. When wheels are build locally they use something like this: 2.6.0a0+gitcbe08da I think we could do something similar here, I will give it a try shortly. |
There is PEP 440 for versions, in a gist a0 stands for first alpha release, + denotes local version. |
1a5d8f0
to
116106b
Compare
There is something wrong with wheel build after upgrade of actions.
Not sure why this errors apart, but apart of that the version code seems ok. |
The build was failing because there are some issues with cbuildhweel when cross compiling to other architectures like arm. |
And I forgot to answer last time, sorry. Here goes the quote from the depreciation notice: And depreciation in this case does not mean warning but a failing workflow. So customers will be forced to upgrade in a month. I've though we could fix it no along with the changes to the build. Additional benefit of v4 are:
So it isn't that bad, and I hoped that the name of the artifact will be changed along with the version number. I will have a look what is going on. |
Thank you! Do we need to cross compile, can't github run arm? I see the arm host are paid even for opensource projects. :( |
The names of the artifacts contain the version right? Won't it be enough to make the upload@v4 work? |
Yeah I see .. I didn't know that .. so we have no choice. |
yes unfortunately :( as of now it's only available for the paid plans. |
No, actually what you did will just change the version of the Python package and not the uploaded artifact name, which is the source of the issue. In this case, do you think there still any benefit of running the git subprocess to get the last commit and maintain a separate version.txt file ? |
That was my initial thought, but I realized it adds complexity. First, shallow builds can be problematic - if you don’t fetch enough commits, you end up without a version number. Using pip install git+http might not help here either, as I believe it also does a shallow copy. And even if pip doesn’t, other developers will inevitably run into issues tryin to speed up a checkout, or simply downloading a tar.gz with source on platforms where git is not available. (Docker?) Maintaining a version.txt file is straightforward, and it has its advantages. For example, if you want to release version 1.3.0, you can easily do so with a one-liner like this: (VAR=1.4.0a0; git tag "$(sed 's/a0$//' version.txt)" && echo "$VAR" > version.txt && git add version.txt && git commit -m "Bump version to $VAR" && git push && git push --tags) This gives you both the release and an alpha release of the next version, which is nice. Achieving the same with tags alone is less clean and not significantly simpler. |
Yes, true, that's my thought as well when I asked. Running a git subprocess will just add complexity without any significant benefit. In this case, let's keep the txt file for the version, and thanks for the command. Should we close this PR then, or do you want to modify it to remove the git subprocess from the |
The current command works with the shallow checkouts, but not with tar.gz, I've fixed that so the local version is empty when git fails. I like the '+git' as it shows clearly what is being installed. Here is an output from uv
|
Crosscompiling to arm64 is still failing, I'm not sure how to turn it off. |
No worries, I will fix it. |
… var. It works without git installed, then local version is empty. The MANIFEST.in was modified so that `python -m build` works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've squashed the commits. There are two as in the meantime I've added support for pip install -e .
it works on macos, can you check if it works on linux too?
Yes, it works on Linux too. Amazing :) I wonder if there is a way to copy the dlls for windows as well! I'll merge this one for now and I'll then fix the CI. |
BTW, what happened to the repair wheel logic ? The CI tests seem to be failing! |
It works on macOS, so I've missed that, sorry. |
hopefully this will work and partially address #78