-
Notifications
You must be signed in to change notification settings - Fork 638
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
Optimize Cmake build slightly #1011
Optimize Cmake build slightly #1011
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
cc @wkpark |
what about python3.9 or 3.8? pytorch nightly build still support python3.8/3.9 (https://download.pytorch.org/whl/nightly/torch/ Of course, I don't expect anyone still uses the old python3.8 for normal use.) |
Should be fine to lower the wheel tag to |
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 for the refactor ! Clean work !
I left few open questions - what do you think?
@@ -9,6 +9,7 @@ concurrency: | |||
|
|||
jobs: | |||
build: | |||
if: github.repository == 'TimDettmers/bitsandbytes' |
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.
can you elaborate on this change? Would this affect PRs from forks?
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.
Yes. I was running exploratory versions of this PR in my own fork, and the docs workflow wouldn't be necessary there.
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.
ok makes sense
@@ -61,7 +47,7 @@ jobs: | |||
environment-file: environment-bnb.yml | |||
use-only-tar-bz2: false | |||
auto-activate-base: true | |||
python-version: ${{ matrix.python-version }} | |||
python-version: "3.10" |
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.
Hmm thinking about it, ideally I think we should add also another "old enough" python version such as 3.8 just to be sure
In the past we had some issues due to a wrong typehint (!) that broke transformers for python 3.8: #859 so I think adding it would circumvents such an issue being on main - what do you think?
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.
I just saw #1010 I think what you said makes sense, we could do the python=3.8 build in another PR - what do you think?
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.
We would add a follow-up step that tests the built libraries on all supported versions, but building the binaries (and wheel) only needs to occur on one version.
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 again !
cc @wkpark all good if we merge this? |
Yes. looks good to me. |
Hey, I am not completely up to speed with everything, but I noticed that all the builds are kicked off even when I just add a documentation commit. That seems pretty wasteful. Wouldn't it make sense to only trigger those workflows in cases where there was a change detected in the csrc directory and otherwise use the last cached one form a parent commit? Not sure how easy/hard that is. Just sth that came to mind. It also doesn't need to be addressed right away, even if it's something we want to do. |
@Titus-von-Koeller great point, actually we can do that in a straightforward manner with GHA, let me open a quick PR and ping you |
This follows up on #908 to make it do less work and simplify things some.
Since we don't actually link against the Python ABI, we only need to build on Python 3.10. CPython 3.10 and newer should happily accept a
cp310
wheel:(#1010 discusses being able to force the wheel tag, so we could build on a newer, faster Python, but still get a cp310 wheel.)