-
Notifications
You must be signed in to change notification settings - Fork 129
BUG: Regression in C blas flags detection #508
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
Comments
I can't reproduce locally, and our CI seems to be working. Can you check the output of |
Here is what I get:
|
How does it compare with before it started failing? |
Sorry for the delay !
|
|
Thanks! The diff is a bit verbose because of the memory locations, but the critical changes seems to be here: +WARNING (pytensor.tensor.blas): Using NumPy C-API based implementation for BLAS functions.
...
blas__ldflags (<class 'str'>)
Doc: lib[s] to include for [Fortran] level-3 blas implementation
- Value: -L/nix/store/fcfbp2iphh271h19m3g0hi5q0x20l8vv-lapack-3/lib -L/nix/store/29jz2fshxr7lf0ny4hb3q3z0jqpqmfz7-blas-3/lib -llapack -llapacke -lblas -lcblas -llapack -llapacke -lblas -lcblas
+ Value: This is most likely caused by #444 CC @lucianopaz |
Oh, thanks for pointing this out. |
I think we should fix it. We didn't want the behavior to change for users |
Ok, understood. We will then wait for the next release to update. |
I’m confused about the two failing tests. The first says that we expect a C implementation of GeMV but we get a python implementation instead? Or the other way around? Does this mean that the lapack link flags are wrong? If that’s the case, should we remove the lapack blas flags from the check our should we fix lapack? The second failure I don’t know |
The test was expecting a CGEMV and gets a GEMV because PyTensor is not finding the blas/lapack (I don't know the difference) flags in the newer version (so the rewrite that introduces the C version doesn't get triggered). @GaetanLepage showed that indeed the new flags are empty and it now gets the usual using numpy impl warning. I think the second test failure comes from the same source. It is supposed to work if some blas Ops get inserted but they are not. |
This might mean that lapack blas link flags are wrong. These are an addition brought in by #444. The quickest patch would be to comment out the lapack condition from the default blas flags function |
@GaetanLepage, does it also break if you set the blas flags to |
When I install |
@ferrine, did you have access to a nixOS machine to try to delve into this issue? |
@GaetanLepage, I've got a patch that seems to be working over at #517. It should fix the two failing tests that you reported. It turns out that they were both caused by empty Your problem seems to run a bit deeper though. I had understood that in the working version you used to have an empty blas__ldflags value but it looks like I misread the diff statement. Your diff says that in the old version you had
But the new
|
@GaetanLepage, we fixed the tests that were failing as a side effect of having empty blas flags, so the nixOS build should work now. The problem is that, when I'm not familiar with nixOS so I'll try to ask a few questions to see if we can improve the user experience for pytensor there. Neither BLAS nor Lapack are build-time dependencies for pytensor. They are only used at runtime, when an Op needs to compile to C code and then get linked to one of those libraries. Once nix installs BLAS and Lapack, can we know where to look for them in the file-system at runtime? If yes, we could apply a similar patch to #517, but keep it nixOS specific, so that BLAS and Lapack are searched in some nix default place. That way, nix users would get to use |
@lucianopaz thank you for taking the time to fix this ! |
Hi! Is my understanding correct that,
In that case we could just pass pytensor one. Is toolchain a required or an optional dependency? I see that pytensor/pytensor/configdefaults.py Lines 397 to 454 in 7ecb9f8
PATH s (ugly baseline: we can patch configdefaults.py ). Do you think that'd make sense?
RE: using pytensor/pytensor/link/c/cmodule.py Lines 2724 to 2726 in 7ecb9f8
I think this works for us too, but what do you think about using e.g. ❯ nix-shell -p blas -p lapack -p pkg-config
❯ pkg-config --libs blas lapack
-L/nix/store/29jz2fshxr7lf0ny4hb3q3z0jqpqmfz7-blas-3/lib -L/nix/store/fcfbp2iphh271h19m3g0hi5q0x20l8vv-lapack-3/lib -lblas -llapack And it prints out sensible errors: ❯ nix-shell -p pkg-config
❯ pkg-config --libs blas
Package blas was not found in the pkg-config search path.
Perhaps you should add the directory containing `blas.pc'
to the PKG_CONFIG_PATH environment variable
No package 'blas' found
Sure, you can even predict that location before the build happens:) Thanks! |
Hi @SomeoneSerge. Thanks so much for your detailed reply! I'm sorry that it took me so long to answer.
Yes, the Ops get are simply symbolic computations.
I'm don't really know nix so I'm a bit lost with how much of this work should go into the nix build and how much of this should go into
This seems related to build time configuration that I was asking above. I think that it would be awesome to have, but it looks like that pep is still a draft. Again, I'm a bit lost with how much of this should happen at the Just to be sure that something actually needs to be changed in Once again, thanks a lot for all of the information in your comment. Let me know if you what your thoughts are on the pkg-config stuff I mentioned above. |
I think the major issues have been fixed, feel free to open new ones with the specific problems still existing (if any) |
Describe the issue:
The following tests fail with
pytensor==2.18.0
:Reproducable code example:
Error message:
PyTensor version information:
Version 2.18.0
Context for the issue:
I am working on updating pytensor from 2.17.3 to 2.18.0 on nixpkgs.
The text was updated successfully, but these errors were encountered: