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

Add Accelerate framework blas__ldflags tests #1056

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

lucianopaz
Copy link
Contributor

@lucianopaz lucianopaz commented Oct 30, 2024

Description

This just adds a test of the Accelerate framework flag to test in blas__ldflags. I haven't written a test because it proved to be a PITA. I've tested it locally and it seems to work. The only thing that I can say is that we should run the test suite on Mac and ensure that the blas__ldflags should be -framework Accelerate -rpath some_path. I written a test to mock this, but I've also added a macos-latest runner that tries to test blas, and also asserts that the blas__ldflags get set to accelerate.

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pytensor--1056.org.readthedocs.build/en/1056/

@ricardoV94
Copy link
Member

The only thing that I can say is that we should run the test suite on Mac and ensure that the blas__ldflags should be -framework Accelerate -rpath some_path.

Running the whole test suite is probably overkill. Can we just pick some smoke tests besides the flag check?

@lucianopaz
Copy link
Contributor Author

Running the whole test suite is probably overkill. Can we just pick some smoke tests besides the flag check?

I suppose that the blas tests

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.09%. Comparing base (6132203) to head (8307af1).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/tensor/blas.py 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1056      +/-   ##
==========================================
+ Coverage   81.95%   82.09%   +0.14%     
==========================================
  Files         182      182              
  Lines       47872    47901      +29     
  Branches     8618     8631      +13     
==========================================
+ Hits        39233    39325      +92     
+ Misses       6472     6410      -62     
+ Partials     2167     2166       -1     
Files with missing lines Coverage Δ
pytensor/link/c/cmodule.py 60.50% <100.00%> (+3.44%) ⬆️
pytensor/tensor/blas.py 66.18% <94.44%> (+0.63%) ⬆️

... and 7 files with indirect coverage changes

@lucianopaz
Copy link
Contributor Author

I'll have to refactor some tests because the extra condition I added doesn't rely on the same file existence logic that the other conditions did, and the mocks I had in place fail there

@lucianopaz lucianopaz force-pushed the framework_mac branch 2 times, most recently from 37c5713 to bacc9df Compare November 1, 2024 06:44
@lucianopaz
Copy link
Contributor Author

I hope to have fixed my mocks now. @maresb, I'm having a dumb problem with the macos-latest runner. I don't know why, the runner doesn't have md5sum and fails to generate the matrix id. Do you have any idea of an alternative that could work on that runner? Locally, I do have both md5 and md5sum, so I have no idea why the runner shouldn't have md5sum.

@lucianopaz
Copy link
Contributor Author

@ricardoV94, I have no idea what went wrong with the single blas test that failed on the mac run. Could you have a look? Is it worth fixing or should we skip that test on Mac?

@ricardoV94
Copy link
Member

@ricardoV94, I have no idea what went wrong with the single blas test that failed on the mac run. Could you have a look? Is it worth fixing or should we skip that test on Mac?

Can you reproduce locally? Seems meaningful

@lucianopaz lucianopaz force-pushed the framework_mac branch 3 times, most recently from 418ec05 to cc614ea Compare November 2, 2024 21:35
@lucianopaz
Copy link
Contributor Author

lucianopaz commented Nov 4, 2024

@ricardoV94, I have no idea what went wrong with the single blas test that failed on the mac run. Could you have a look? Is it worth fixing or should we skip that test on Mac?

Can you reproduce locally? Seems meaningful

You were totally right, @ricardoV94! This small test turned out to indicate that the link flags were wrong and that pytensor was not compiling against blas at all! There were a bunch of problems, the main one was that pytensor was programed with GNU C compiler, gcc, in mind. This only has -l, -L and -I flags for linking libraries, adding library search dirs and hearder include dirs. clang adds the notion of -framework, that is the same as implicitly adding a bunch of -l, -L and -I flags to the compilation command. The problem was that pytensor actually tried to enforce all blas flags to follow gcc convention, and failed horribly when faced with -framework Accelerate. I had to add a hackish solution to work around the limitations. A proper solution would be to add a c_frameworks to the Ops and handle that in the C linkers, but that would involve a massive refactor that is completely not worth the effort. At the moment, we only really want to handle Accelerate. If the situation changes in the future, we will have to revise this.

@lucianopaz
Copy link
Contributor Author

@ricardoV94, the codecov patch difference isn't significant. The changes I made removed coverage from a couple of lines, and I don't think that it's worth it to prevent the merge just because of that.

@maresb
Copy link
Contributor

maresb commented Nov 4, 2024

The silent failures should be resolved by mamba-org/setup-micromamba#241

@lucianopaz lucianopaz merged commit e655429 into pymc-devs:main Nov 4, 2024
61 of 62 checks passed
@lucianopaz lucianopaz deleted the framework_mac branch November 4, 2024 12:10
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.

pytensor and blas problems on on MacOS 15 Sequoia with Apple Silicon
3 participants