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

Make BLAS flags check lazy and more actionable #1165

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jan 23, 2025

It replaces the old eager warning that does not actually apply by a lazy yet more informative and actionable warning.

This however means users won't notice something is wrong with their installation until PyTensor actually accesses the config.blas__ldflags to decide on some behavior.

These flags are accessed to either not trigger the introduction of more efficient COps (CGEMv, sparse Usmm, ...), or to use the Python or C implementation for Ops that have both (Dot22, BatchedDot, ...).

The new warning will happen only when a function is compiled that triggers those code-paths.


Technically the old warning was wrong:
The specific warning message was about Ops that might use the alternative blas_headers, which rely on the Numpy C-API.

However, regular PyTensor user has not used this for a while. The only Op that would use C-code with this alternative headers is the GEMM Op which is not included in current rewrites. Instead Dot22 or Dot22Scalar are introduced, which refuse to generate C-code altogether if the BLAS flags are missing.


This complements #1161 and makes PyTensor a breeze to import compared to before.
Closes #1160 (although we could still try to speed the check up)

@jessegrabowski
Copy link
Member

Love it. I suggest also mentioning that you can set the mode argument in .pytensorrc, so that's it becomes the global default. This would benefit users who are adamantly stanning pip (and should thus always use a non-C backend)

@ricardoV94
Copy link
Member Author

Love it. I suggest also mentioning that you can set the mode argument in .pytensorrc, so that's it becomes the global default. This would benefit users who are adamantly stanning pip (and should thus always use a non-C backend)

Maybe update and link to https://pytensor.readthedocs.io/en/latest/troubleshooting.html#test-blas ?

@ricardoV94 ricardoV94 changed the title Make Blas flags check lazy Make Blas flags check lazy and more actionable Jan 23, 2025
@ricardoV94 ricardoV94 force-pushed the make_blas_check_lazy branch from 89af81b to 1fd792c Compare January 23, 2025 12:16
@ricardoV94 ricardoV94 changed the title Make Blas flags check lazy and more actionable Make BLAS flags check lazy and more actionable Jan 23, 2025
pytensor/link/c/cmodule.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.27%. Comparing base (6bdfbae) to head (b9cc961).
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1165      +/-   ##
==========================================
- Coverage   82.27%   82.27%   -0.01%     
==========================================
  Files         186      186              
  Lines       48023    48004      -19     
  Branches     8630     8623       -7     
==========================================
- Hits        39513    39495      -18     
- Misses       6347     6351       +4     
+ Partials     2163     2158       -5     
Files with missing lines Coverage Δ
pytensor/link/c/cmodule.py 60.87% <100.00%> (+0.47%) ⬆️
pytensor/tensor/blas_headers.py 70.78% <100.00%> (+1.89%) ⬆️

... and 11 files with indirect coverage changes

@ricardoV94 ricardoV94 force-pushed the make_blas_check_lazy branch from 1fd792c to 0aa045b Compare January 23, 2025 14:08
@ricardoV94 ricardoV94 added bug Something isn't working backend compatibility labels Jan 23, 2025
@ricardoV94 ricardoV94 force-pushed the make_blas_check_lazy branch from 0aa045b to c3c0b2c Compare January 23, 2025 14:11
@ricardoV94 ricardoV94 removed bug Something isn't working backend compatibility labels Jan 23, 2025
directly).
this, set the value of ``blas__ldflags`` as the empty string.
Depending on the kind of matrix operations your PyTensor code performs,
this might slow some things down (vs. linking with BLAS directly).

2) You can install the default (reference) version of BLAS if the NumPy version
Copy link
Member

Choose a reason for hiding this comment

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

Do we want points (2) and (3) in here anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, it's the most hardcore version to get it up and running, why not mention it?

Copy link
Member

Choose a reason for hiding this comment

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

fair enough!

@ricardoV94 ricardoV94 force-pushed the make_blas_check_lazy branch 2 times, most recently from 1a71f28 to 032fe15 Compare January 23, 2025 15:19
@ricardoV94 ricardoV94 marked this pull request as ready for review January 23, 2025 15:38
@ricardoV94 ricardoV94 force-pushed the make_blas_check_lazy branch from 032fe15 to c8c4660 Compare January 23, 2025 17:43
It replaces the old warning that does not actually apply by a more informative and actionable one.

This warning was for Ops that might use the alternative blas_headers, which rely on the Numpy C-API.

However, regular PyTensor user has not used this for a while. The only Op that would use C-code with this alternative headers is the GEMM Op which is not included in current rewrites. Instead Dot22 or Dot22Scalar are introduced, which refuse to generate C-code altogether if the blas flags are missing.
@ricardoV94 ricardoV94 force-pushed the make_blas_check_lazy branch from c8c4660 to b9cc961 Compare January 24, 2025 08:19
@maresb
Copy link
Contributor

maresb commented Jan 24, 2025

This is awesome!!!

Note to self: I'll need to rework the conda-forge tests since that assumes eager BLAS

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jan 24, 2025

This is awesome!!!

Note to self: I'll need to rework the conda-forge tests since that assumes eager BLAS

Doesn't it explicitly check the pytensor.config.blas__ldflags? that will trigger the logic

@maresb
Copy link
Contributor

maresb commented Jan 24, 2025

I run python -c "import pytensor.configdefaults; import pytensor.tensor". Thanks for the suggestion, I'll just change it to that.

Copy link
Contributor

@Ch0ronomato Ch0ronomato left a comment

Choose a reason for hiding this comment

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

Yup, this makes a lot of sense as other backends get developed.

@ricardoV94 ricardoV94 merged commit cb428f2 into pymc-devs:main Jan 28, 2025
63 of 64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checking for blas flags has a high import cost on the library
5 participants