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

Use setuptools-scm #666

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Use setuptools-scm #666

merged 2 commits into from
Dec 16, 2024

Conversation

ur4t
Copy link
Contributor

@ur4t ur4t commented Dec 16, 2024

No description provided.

Copy link
Collaborator

@yzh119 yzh119 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@yzh119 yzh119 merged commit 08405a5 into flashinfer-ai:main Dec 16, 2024
@ur4t
Copy link
Contributor Author

ur4t commented Dec 16, 2024

@yzh119 Version in documentation is broken. I will fix it later.

ur4t added a commit to ur4t/flashinfer-nightly that referenced this pull request Dec 16, 2024
@@ -1 +0,0 @@
0.1.6
Copy link
Member

Choose a reason for hiding this comment

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

^QQ Why remove this?

Copy link
Member

Choose a reason for hiding this comment

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

Should we remain compatible with the previous version? @ur4t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I believe it is no longer useful:

  • Inside our project, all usage are replaced.
  • In Python packages version info is generated by setuptools-scm and stored in flashinfer._version.
  • For C++ users, usually the whole repo is cloned and they have much more freedom to check versions.

Copy link
Member

Choose a reason for hiding this comment

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

In Python packages version info is generated by setuptools-scm and stored in flashinfer._version.

Could you explain this in detail, because the latest main seems to be missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain this in detail, because the latest main seems to be missing?

https://setuptools-scm.readthedocs.io/en/latest/usage/

TLDR: When packaged as wheels/sdists or installed via pip install -e, flashinfer/_version.py is generated which contains all version info (base version and git hash, even cuda version and torch version in AOT mode).

This is enough for most usages I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is the information for the base version obtained from?

If the repo is on a tag such as 0.1.7, the wheels will be flashinfer-0.1.7-cp38-abi3-linux_x86_64.whl(AOT mode) or flashinfer-0.1.7-py3-none-any.whl(JIT mode).

It the repo is not on a tag but a commit after 0.1.7, a local version +gXXXXXXX is attached, such as flashinfer-0.1.7+gXXXXXXX-cp38-abi3-linux_x86_64.whl.

It the repo is not clean, .dYYYYMMDD is attached to +gXXXXXXX.

Additional info like CUDA version and Torch version is collected inside setup.py when the wheel is built in AOT mode and attached like 0.1.7(+gXXXXXXX(.dYYYYMMDD)).cu124torch2.5, but I am afraid it is not proper because 0.1.7.cu124torch2.5 is obviously wrong.

Copy link
Member

Choose a reason for hiding this comment

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

0.1.6+commit_hash.cu124torch2.5 is ok
0.1.6+commit_hash+cu124torch2.5 is wrong
you can check it with twine check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0.1.6+commit_hash.cu124torch2.5 is ok 0.1.6+commit_hash+cu124torch2.5 is wrong you can check it with twine check

Yes. + as indicator of local version should only appear once. 0.1.7.cu124torch2.5 is wrong because only 0.1.7.postX or 0.1.7.devX is allowed.

AFAIK, Pypi does not allow to upload wheels with local version. So which version of Torch (and underlying CUDA) should we use is also a problem.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, Pypi does not allow to upload wheels with local version. So which version of Torch (and underlying CUDA) should we use is also a problem.

I know this. ref #666 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to admit that setuptools-scm is good but not suitable for Github actions hierarchy:

  • setuptools-scm requires more git metadata but actions/checkout perfers shallow clone.
  • setuptools-scm is not that straightforward as version.txt.

@ur4t ur4t deleted the setuptools-scm branch December 17, 2024 04:44
ur4t added a commit to ur4t/flashinfer that referenced this pull request Dec 17, 2024
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.

3 participants