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

Support for 32bit architectures #110

Merged
merged 2 commits into from
Sep 9, 2022
Merged

Conversation

avalentino
Copy link
Contributor

The numba 'parallel' target is not currently supported on 32 bit hardware.
Fallback to the sequential version.

The numba 'parallel' target is not currently supported on 32 bit hardware.
Fallback to the sequential version.
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2022

Codecov Report

Merging #110 (350507b) into main (2ca177f) will decrease coverage by 5.36%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
- Coverage   80.58%   75.22%   -5.37%     
==========================================
  Files           4        4              
  Lines         103      113      +10     
==========================================
+ Hits           83       85       +2     
- Misses         20       28       +8     
Flag Coverage Δ
unittests 75.22% <16.66%> (-5.37%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
resampy/core.py 70.31% <16.66%> (-9.32%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bmcfee
Copy link
Owner

bmcfee commented Aug 25, 2022

Thanks for noting this. I generally support the idea of catching this case with a warning and falling back to serial, but I'm not sure that doing this by probing the architecture is the best way to accomplish it. If/when in the future, numba does fix this, we'd have to rewrite this check and push a new version.

Would it be possible to do this by a try-except instead? Is there a specific exception that is thrown by parallel compilation on 32bit architectures?

Additionally, it would be great if we could have a CI test environment for this. Github actions doesn't provide this directly, but this comment suggests that it could be emulated with a docker container.

@avalentino
Copy link
Contributor Author

avalentino commented Aug 26, 2022

@bmcfee I have update the patch to make a dynamic detection of the numba 'parallel' support.
Regarding the 32bit environment, it is not clear to me how to approach.
Probably it is enough to use the 'architecture' parameter of setup-python.

By the way, please not that I have packaged resampy for debian and the package is now tested on several architectures: https://ci.debian.net/packages/r/resampy.

@bmcfee
Copy link
Owner

bmcfee commented Sep 2, 2022

@bmcfee I have update the patch to make a dynamic detection of the numba 'parallel' support.

Thanks!

Regarding the 32bit environment, it is not clear to me how to approach. Probably it is enough to use the 'architecture' parameter of setup-python.

I'm not sure if that will do it, but apparently you can force anaconda to use 32-bit binaries: https://stackoverflow.com/a/33711433

Numpy, scipy, and numba all have anaconda builds on linux-32, so I think this will actually work. I expect the easiest way to set this up is to duplicate the ci-minimal action to create a new one (CI-32bit), add the following to the top:

env:
  CONDA_FORCE_32BIT: 1

and bring the coverage bits back in from the main CI so that we get a coverage report on the exception handler.

By the way, please not that I have packaged resampy for debian and the package is now tested on several architectures: https://ci.debian.net/packages/r/resampy.

Awesome - thanks for this!

@avalentino
Copy link
Contributor Author

avalentino commented Sep 2, 2022

Unfortunately the newest numba available for 32bit linux is v0.41.
Also 32bit wheels seems to be not available for numba>=0.53.

Do you have any other idea to setup a 32bit environment?
Otherwise I will remove the ci-32bit.yml form this PR since it is not working.

To me if things start to become too complex then probably it does not worth the effort.
We could just manually test resampy on 32bit docker image before a new release.

@bmcfee
Copy link
Owner

bmcfee commented Sep 5, 2022

Unfortunately the newest numba available for 32bit linux is v0.41. Also 32bit wheels seems to be not available for numba>=0.53.

Do you have any other idea to setup a 32bit environment? Otherwise I will remove the ci-32bit.yml form this PR since it is not working.

To me if things start to become too complex then probably it does not worth the effort. We could just manually test resampy on 32bit docker image before a new release.

Thanks for trying this out!

The only other idea I have for this is in the previous comment #110 (comment) , but I agree that it seems like more of a hassle than it's worth.

This otherwise seems fine to me - should we merge?

@avalentino
Copy link
Contributor Author

The only other idea I have for this is in the previous comment #110 (comment) , but I agree that it seems like more of a hassle than it's worth.

do you mean using docker?
I have no experience with docker in GHA
I need to make some test

This otherwise seems fine to me - should we merge?

I have removed commits regarding CI.
It is now ready to merge.

Regarding a possible implementation of CI for 32bit system (via docker or any other solution), IMHO it could be implemented in a separate PR if you are still interested.

@bmcfee bmcfee merged commit 5c283f0 into bmcfee:main Sep 9, 2022
@bmcfee bmcfee added this to the 0.4.1 milestone Sep 9, 2022
@avalentino avalentino deleted the bugfix/32bit branch December 14, 2023 07:22
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.

3 participants