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

Optimize Cmake build slightly #1011

Merged
merged 5 commits into from
Feb 2, 2024

Conversation

akx
Copy link
Contributor

@akx akx commented Feb 1, 2024

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:

$ docker run -it --platform=linux/amd64 python:3.12 sh -c 'pip debug --verbose | grep cp310'
[...]
  cp310-abi3-manylinux1_x86_64
  cp310-abi3-linux_x86_64

(#1010 discusses being able to force the wheel tag, so we could build on a newer, faster Python, but still get a cp310 wheel.)

Copy link

github-actions bot commented Feb 1, 2024

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.

@akx akx changed the title Tweak/optimize Cmake build slightly Optimize Cmake build slightly Feb 1, 2024
@akx
Copy link
Contributor Author

akx commented Feb 1, 2024

cc @wkpark

@wkpark
Copy link
Contributor

wkpark commented Feb 1, 2024

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.)

@akx
Copy link
Contributor Author

akx commented Feb 1, 2024

Should be fine to lower the wheel tag to cp38 eventually!

Copy link
Collaborator

@younesbelkada younesbelkada left a 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'
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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"
Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks again !

@younesbelkada
Copy link
Collaborator

cc @wkpark all good if we merge this?

@wkpark
Copy link
Contributor

wkpark commented Feb 2, 2024

all good if we merge this?

Yes. looks good to me.

@Titus-von-Koeller
Copy link
Collaborator

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.

@younesbelkada
Copy link
Collaborator

@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

@younesbelkada younesbelkada merged commit 5c0ba85 into TimDettmers:main Feb 2, 2024
6 checks passed
@younesbelkada
Copy link
Collaborator

@Titus-von-Koeller : #1015

@akx akx deleted the tweak-cmake-build-1 branch February 2, 2024 06:30
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.

None yet

4 participants