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

scikit-sparse install problems don't fail meaningfully #239

Open
paulthebaker opened this issue Dec 9, 2020 · 1 comment
Open

scikit-sparse install problems don't fail meaningfully #239

paulthebaker opened this issue Dec 9, 2020 · 1 comment
Assignees
Labels
bug dependencies Pull requests that update a dependency file

Comments

@paulthebaker
Copy link
Member

@siyuan-chen found an issue in a fresh enterprise install using pip.

scikit-sparse (sks) depends on the SuiteSparse C library. If SuiteSparse isn't installed, sks can still install "successfully", but it will throw errors when used.

In LogLikelihood.__call__() there's a cholesky call inside a try / except that catches all errors. This means that if sks isn't linked to SuiteSparse correctly, the sks error will send the code to the except statement. LogLiklihood will return -inf when it should crash and exit. There's no indication to the user what went wrong.

The install problem can happen because requirements.txt installs sks with the --no-binary option. This assumes the user has an existing install of SuiteSparse. Installing the packaged binary of sks will use a generic BLAS, which is slower than a locally optimized version. If you install packaged binaries of numpy and scipy via pip, they won't used optimized BLAS either.

There are few things to do:

  • additionally check sks linking when we check that it is installed and give a meaningful error message
  • change the requirements.txt to use the packaged binary version of sks. A pip install is already using generic BLAS for numpy... 🤷
  • specify which errors that try / except is intended to catch, so the code exits on others. I think we want it to return -inf and continue if Sigma is singular
@paulthebaker paulthebaker added bug dependencies Pull requests that update a dependency file labels Dec 9, 2020
@paulthebaker paulthebaker self-assigned this Dec 9, 2020
@jellis18
Copy link
Contributor

This is somewhat related to #257 and #258 and will somewhat be solved by this PR I could possible add a check in here so the install will fail properly if the linked libraries aren't found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug dependencies Pull requests that update a dependency file
Projects
None yet
Development

No branches or pull requests

2 participants