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 big endian platform by providing new ops implementation #559

Merged
merged 12 commits into from
Dec 21, 2021
14 changes: 13 additions & 1 deletion thinc/backends/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ def _import_extra_cpu_backends():
from thinc_apple_ops import AppleOps
except ImportError:
pass
try:
from thinc_bigendian_ops import BigEndianOps
except ImportError:
pass


def get_ops(name: str, **kwargs) -> Ops:
Expand All @@ -91,10 +95,15 @@ def get_ops(name: str, **kwargs) -> Ops:
cls: Optional[Callable[..., Ops]] = None
if name == "cpu":
_import_extra_cpu_backends()

cls = ops_by_name.get("apple", ops_by_name.get("numpy"))

if "bigendian" in ops_by_name:
cls = ops_by_name.get("bigendian", ops_by_name.get("numpy"))

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cls = ops_by_name.get("apple", ops_by_name.get("numpy"))
if "bigendian" in ops_by_name:
cls = ops_by_name.get("bigendian", ops_by_name.get("numpy"))
cls = ops_by_name.get("numpy")
cls = ops_by_name.get("apple", cls)
cls = ops_by_name.get("bigendian", cls)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is where a ranking from the ops classes themselves would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with suggested change and retested.

else:
cls = ops_by_name.get(name)

if cls is None:
raise ValueError(f"Invalid backend: {name}")

Expand All @@ -113,6 +122,9 @@ def get_array_ops(arr):
def use_ops(name: str, **kwargs):
"""Change the backend to execute on for the scope of the block."""
current_ops = get_current_ops()
# avoid fallback to base NumpyOps if on big endian platform
if current_ops.name == "bigendian" and name == "numpy":
name = current_ops.name
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we've removed all the use_ops("numpy") from our code, so I think I'd rather have at most a warning/error rather than having use_ops silently not do what you just told it to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the highlighted code... At best, this was my attempt to keep an explicit use_ops("numpy") specification from reverting back to numpy ops when explicitly requested from outside of thinc. It is best not to ignore an explicit request.

set_current_ops(get_ops(name, **kwargs))
try:
yield
Expand Down
7 changes: 7 additions & 0 deletions thinc/tests/backends/test_ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,13 @@ def test_get_ops():
assert isinstance(get_ops("cpu"), AppleOps)
except ImportError:
assert isinstance(get_ops("cpu"), NumpyOps)
# If BigEndian ops are available, "cpu" should return BigEndianOps or
# NumpyOps otherwise.
try:
from thinc_bigendian_ops import BigEndianOps
assert isinstance(get_ops("cpu"), BigEndianOps)
except ImportError:
assert isinstance(get_ops("cpu"), NumpyOps)
with pytest.raises(ValueError):
get_ops("blah")
ops = Ops(numpy)
Expand Down