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

Added install requirements to setup #488

Merged

Conversation

ccoulombe
Copy link
Contributor

When installing or distributing, ensure needed requirements are present.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

@github-actions github-actions bot closed this Dec 29, 2023
@TimDettmers
Copy link
Collaborator

Thanks for this contribution! When working with setup.py in other projects, I sometimes noticed that my pytorch gets uninstalled and a different version installed. I think this only happens if a specific version is pinned. Is there any other scenario where this addition could cause the wrong pytorch version to be installed? I could imagine a new OS/VM and one installed bitsandbytes first and it installs a different pytorch version as one intended.

@Titus-von-Koeller @younesbelkada, what is your opinion?

@TimDettmers TimDettmers reopened this Jan 1, 2024
@TimDettmers TimDettmers added high priority (first issues that will be worked on) Low Risk Risk of bugs in transformers and other libraries labels Jan 1, 2024
@akx
Copy link
Contributor

akx commented Jan 2, 2024

@TimDettmers If I may chip in, that shouldn't be an issue here – there is no version requirement here, so any version of torch installed should satisfy the requirement. (Of course setup.py in itself is antiquated, and should be replaced with a PEP 517 build backend, but I see there's parallel work to do cmake things and so on, so that'd probably conflict.)

That said, pandas and matplotlib should not be specified as requirements here, as commented below!

There's also an argument for not requiring Scipy by default: #948

setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@akx akx left a comment

Choose a reason for hiding this comment

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

Not good to hard-require pandas and matplotlib.

setup.py Outdated Show resolved Hide resolved
Co-authored-by: Aarni Koskela <[email protected]>
@ccoulombe
Copy link
Contributor Author

@TimDettmers Pip may install a different pytorch version if another package installed in the same virtual environement as bitsandbyte require (strictly) a different version. Otherwise, since the requirement is torch, it means that no specific version is required by bitsandbytes.

On HPC systems where administrators maintains a wheelhouse, or to simply avoid user issues when installing/using packages, it is easier to have the main package request its dependencies.

As for matplotlib or pandas, I've accepted @akx suggestion

@Titus-von-Koeller Titus-von-Koeller merged commit 1e64210 into bitsandbytes-foundation:main Jan 23, 2024
@Titus-von-Koeller
Copy link
Collaborator

You're right, the build part of bnb might see overhauls soon, but I don't see any reason not to merge this. Makes sense. Thanks for your contribution, really appreciated 🤗

@akx
Copy link
Contributor

akx commented Jan 24, 2024

This was mis-merged somehow 😞 (see #981, #983).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority (first issues that will be worked on) Low Risk Risk of bugs in transformers and other libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants