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 scipy to requirements.txt #525

Merged
merged 2 commits into from Jan 2, 2024
Merged

Conversation

dulalbert
Copy link
Contributor

Added scipy to requirements.txt as it is used but not added to requirements

Added scipy to requirements.txt as it is used but not added to requirements
@shadeMe
Copy link
Contributor

shadeMe commented Jul 11, 2023

+1

This breaks the conditional importing of bitsandbytes, i.e.,

try:
    import bitsandbytes

    has_bitsandbytes = True
except ImportError:
    bitsandbytes = None  # type: ignore
    has_bitsandbytes = False

has_bitsandbytes will be False even if the user has the library installed (but not scipy) since the ImportError gets masked.

EDIT: According to the official docs, the requirement should also be added to install_requires.

@danieldk
Copy link

I think that the dependency should also be added to the install_requires, otherwise scipy doesn't get installed when pip install-ing bitsandbytes.

@dulalbert dulalbert mentioned this pull request Jul 18, 2023
@nazarov-yuriy
Copy link

@TimDettmers
Could we merge it?
Apparently 8a20cd8 is not enough.

@rasbt
Copy link
Contributor

rasbt commented Aug 8, 2023

Should be a quick merge in case this got forgotten 😅

@mnoukhov
Copy link

mnoukhov commented Sep 2, 2023

@TimDettmers is this ok to merge?

8a20cd8 is indeed not enough. Anyone installing bitsandbytes through pip e.g. pip install bitsandbytes will not install scipy and code will error.
scipy needs to be added to the install_requires

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.

@mnoukhov
Copy link

This still needs to be addressed. Easy fix, should be merged

@TimDettmers
Copy link
Owner

Sorry for the delay on this. Thanks for contributing this fix!

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

7 participants