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

Changing package import logic #12

Closed
rtobar opened this issue Oct 25, 2019 · 5 comments
Closed

Changing package import logic #12

rtobar opened this issue Oct 25, 2019 · 5 comments

Comments

@rtobar
Copy link
Contributor

rtobar commented Oct 25, 2019

This issue is to change the package's import logic. At the moment the package fails to import in platforms where no hardware support is found (i.e., anything other than Intel SSE4.2-capable CPUs) unless an environment variable, specified beforehand, instructs the package to expose its software implementation instead.

This package initially contained only an Intel-based crc32c implementation, but with time a software implementation was incorporated but the importing logic was maintained for backwards compatibility. With the advent of the software implementation (#4) the importing logic has been highlighted as being not necessarily ideal (#5 and #11).

An improved logic, and what most users seem to expect, would work as follow: by default a hardware-based implementation would be offered. If this cannot be offered, the software implementation would be offered instead. This way the package would never fail to import. This would imply that the CRC32C_SW_MODE variable would become deprecated (we could issue such a deprecation warning upon seeing it). Additionally I would still like it to be clear when a hardware implementation is exposed and when not -- maybe this can be done by attaching an attribute to the crc32c function, or at the package level. This way people relying on a fast, hardware-based implementation can still check if this is the case and act accordingly.

Since this would be a breaking change (with not too grave consequences though), this would need a bump on the package's major version.

@ossdev07
Copy link

Hi @rtobar ,
Thanks for picking up on this issue so early.
How soon can we expect a release with the necessary changes ? Any tentative timeline ?

@rtobar
Copy link
Contributor Author

rtobar commented Oct 25, 2019

I probably won't be pressing too hard on this on the near future -- say until December or so, but who knows. I created the ticket more as a reminder to do it, not necessarily because I was planning to do it soon.

I'm happy to review PRs though, along the lines of what I described above.

@rtobar
Copy link
Contributor Author

rtobar commented Oct 29, 2019

BTW, until this is implemented something like the following be used to avoid import errors:

import os

os.environ['CRC32C_SW_MODE'] = 'auto'
import crc32c

@rtobar
Copy link
Contributor Author

rtobar commented Nov 26, 2019

A first version of the new logic is now available in the master branch. Now the software implementation is used by default as a fallback, unless the CRC32C_SW_MODE environment variable is set to none, in which case the old behavior takes place.

@ossdev07 please have a try and let me know how this works for you. After some more testing I'll be cutting a new crc32c version (probably next week though) with this change.

@rtobar
Copy link
Contributor Author

rtobar commented Dec 2, 2019

crc32c version 2.0 now released in PyPI contains this change, closing issue now.

@rtobar rtobar closed this as completed Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants