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

feat: test thread-safety #65

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jkrimmer
Copy link
Contributor

This PR adds a basic test for the thread-safety of FINUFFT in julia. According to #63 , this test fails on MacOS but not on Windows or Linux.

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.83%. Comparing base (2a54a96) to head (e87be1d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   58.83%   58.83%           
=======================================
  Files           8        8           
  Lines         566      566           
=======================================
  Hits          333      333           
  Misses        233      233           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ludvigak
Copy link
Owner

Thanks for the PR!
I don't have a Mac to test this, and I am not convinced that we should add the test to the standard test suite unless we can actually make it fail in the CI. (Although I realize that this isn't the case for the cuFINUFFT tests either.)
Maybe we can put this and other tests that catch thread issues under test/extras for now?

@jkrimmer jkrimmer marked this pull request as draft August 23, 2024 16:42
This way, we should be able to reproduce the failing threading tests
@jkrimmer
Copy link
Contributor Author

jkrimmer commented Aug 23, 2024

I have now modified the CI to run the tests also on an arm64-Mac runner on julia 1.10. Surprisingly, the CI can neither reproduce the issues #63 nor #64 ... Probably, the issue is related to my M3 chip then.
Still, I can move the new test to test/extras if you prefer! :)

@jkrimmer jkrimmer marked this pull request as ready for review August 23, 2024 16:56
@jkrimmer jkrimmer marked this pull request as draft August 23, 2024 19:55
@jkrimmer
Copy link
Contributor Author

I forgot to make sure the tests actually run with multiple threads. Now, the segmentation fault on the Mac is back - besides additional errors which I have to investigate further!

@jkrimmer
Copy link
Contributor Author

Actually, this one seems to be ready - I have rerun the failing test on Ubuntu and it went smoothly. Now, only the multithreading tests on MacOS fail, probably due to some OpenMP issue that I have not been able to identify yet.

@jkrimmer jkrimmer marked this pull request as ready for review September 10, 2024 16:47
@ludvigak
Copy link
Owner

Do the tests pass if you sync this PR with master, so that it includes your fix for FFTW thread safety?

@jkrimmer jkrimmer marked this pull request as draft September 11, 2024 21:02
@jkrimmer
Copy link
Contributor Author

I have played around with some more CI tweaks; however, the FFTW thread-safety adaptations do not help with the issues on MacOS...

@jkrimmer jkrimmer marked this pull request as ready for review September 11, 2024 21:58
@ludvigak
Copy link
Owner

Ok. I don't think we should merge this as long as it's failing in the CI. We could hold off with it until a fix is in place, and then merge it in so that we can detect any future regression on the thread safety. Or just disable the test on MacOS for now, but then we would need to remember to turn it back on.

@jkrimmer
Copy link
Contributor Author

Sounds good! Then, let's keep this PR open until the MacOS issue has been resolved.

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.

2 participants