Skip to content

Conversation

@chunnienc
Copy link
Collaborator

@chunnienc chunnienc commented Jan 9, 2023

This PR also updates the atan kernel's behavior: it can take i32/bool tensor and return the output in the same dtype.
This change is to make the behavior of wasm kernels align with cpu and webgl kernels, although the outputs of these kernels are subtly different now.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@chunnienc chunnienc marked this pull request as ready for review January 9, 2023 06:01
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @chunnienc)


tfjs-backend-wasm/src/cc/kernels/Acosh.cc line 51 at r1 (raw file):

      break;
    case DType::int32:
      unary_i32(x_id, out_id, acosh_impl<int>);

I am not sure what would i32 and bool result represent for acos and acosh, are there any tests in tensorflow that target these output dtypes?

Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

Looks almost ready to be merged. I just have a few comments on the output types of acos and acosh. Thanks!

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)


tfjs-backend-wasm/src/cc/kernels/Acosh.cc line 51 at r1 (raw file):
Bool does not seem to be supported by TF Acos:

>>> ibool = tf.constant([True, False])
>>> tf.raw_ops.Acos(x=ibool)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/msoulanille/.local/lib/python3.10/site-packages/tensorflow/python/util/tf_export.py", line 412, in wrapper
    return f(**kwargs)
  File "/home/msoulanille/.local/lib/python3.10/site-packages/tensorflow/python/ops/gen_math_ops.py", line 184, in acos
    _ops.raise_from_not_ok_status(e, name)
  File "/home/msoulanille/.local/lib/python3.10/site-packages/tensorflow/python/framework/ops.py", line 7209, in raise_from_not_ok_status
    raise core._status_to_exception(e) from None  # pylint: disable=protected-access
tensorflow.python.framework.errors_impl.InvalidArgumentError: Value for attr 'T' of bool is not in the list of allowed values: bfloat16, half, float, double, int8, int16, int32, int64, complex64, complex128
	; NodeDef: {{node Acos}}; Op<name=Acos; signature=x:T -> y:T; attr=T:type,allowed=[DT_BFLOAT16, DT_HALF, DT_FLOAT, DT_DOUBLE, DT_INT8, DT_INT16, DT_INT32, DT_INT64, DT_COMPLEX64, DT_COMPLEX128]> [Op:Acos]

However, int is supported:

>>> input = tf.constant([0, 1, -1], dtype='int32')
>>> tf.raw_ops.Acos(x=input)
<tf.Tensor: shape=(3,), dtype=int32, numpy=array([1, 0, 3], dtype=int32)>

Docs: https://www.tensorflow.org/api_docs/python/tf/raw_ops/Acos#returns

For Acosh, bool is not supported (similar error message), and surprisingly, int is also not supported.
Docs: https://www.tensorflow.org/api_docs/python/tf/raw_ops/Acosh


tfjs-backend-wasm/src/cc/kernels/Atan.cc line 51 at r2 (raw file):

      break;
    case DType::int32:
      unary_i32(x_id, out_id, atan_impl<int>);

Nice catch. I think I missed that the output type is the same as the input type in my last review.
https://www.tensorflow.org/api_docs/python/tf/raw_ops/Atan

Code quote:

      unary_i32(x_id, out_id, atan_impl<int>);

@chunnienc
Copy link
Collaborator Author

chunnienc commented Jan 9, 2023

Collaborator

No, I'd say the current behavior of acos and acosh on int32 and bool are undefined, and our cpu and webgl can return different results for int32 and bool types: https://jsfiddle.net/nas437md/ (you can try to modify the dtype to int32 and bool). What I'm trying to do is to make the wasm kernel "do something" instead of throwing an error, so that it somehow behaves simiarly across different kernels.

These are not the only kernels/ops having this issue, I was playing around with different backend recently and there are various problems with int32 and bool type:
1.Backends returns different results (like acos and acosh in cpu and webgl)
2. The coverage are different (like neg - cpu and webgl accept bool dtype, but there is no wasm implementation)
3. Not well documented - our public API doc has no information about how these ops work with int32 and bool type. (some ops like cos do mention 'float32' is the only acceptable type in the document)
4. Not aligned with the python TF API, and not documented.

Sadly, there are no tests for behaviors with unexpected dtypes now. I'm still thinking about various way to deal with this issue.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Yeah, I agree there are definitely inconsistent right now with our backends and thank you for digging into this.
In general I think if WASM can mimic TF CPU behavior would be great, typically GPU behave slightly differently even for TF, due to GPU shader cannot throw errors.

Reviewable status: 0 of 1 approvals obtained

@chunnienc chunnienc closed this Jan 9, 2023
@chunnienc chunnienc reopened this Jan 9, 2023
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille)


tfjs-backend-wasm/src/cc/kernels/Acosh.cc line 51 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

Bool does not seem to be supported by TF Acos:

>>> ibool = tf.constant([True, False])
>>> tf.raw_ops.Acos(x=ibool)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/msoulanille/.local/lib/python3.10/site-packages/tensorflow/python/util/tf_export.py", line 412, in wrapper
    return f(**kwargs)
  File "/home/msoulanille/.local/lib/python3.10/site-packages/tensorflow/python/ops/gen_math_ops.py", line 184, in acos
    _ops.raise_from_not_ok_status(e, name)
  File "/home/msoulanille/.local/lib/python3.10/site-packages/tensorflow/python/framework/ops.py", line 7209, in raise_from_not_ok_status
    raise core._status_to_exception(e) from None  # pylint: disable=protected-access
tensorflow.python.framework.errors_impl.InvalidArgumentError: Value for attr 'T' of bool is not in the list of allowed values: bfloat16, half, float, double, int8, int16, int32, int64, complex64, complex128
	; NodeDef: {{node Acos}}; Op<name=Acos; signature=x:T -> y:T; attr=T:type,allowed=[DT_BFLOAT16, DT_HALF, DT_FLOAT, DT_DOUBLE, DT_INT8, DT_INT16, DT_INT32, DT_INT64, DT_COMPLEX64, DT_COMPLEX128]> [Op:Acos]

However, int is supported:

>>> input = tf.constant([0, 1, -1], dtype='int32')
>>> tf.raw_ops.Acos(x=input)
<tf.Tensor: shape=(3,), dtype=int32, numpy=array([1, 0, 3], dtype=int32)>

Docs: https://www.tensorflow.org/api_docs/python/tf/raw_ops/Acos#returns

For Acosh, bool is not supported (similar error message), and surprisingly, int is also not supported.
Docs: https://www.tensorflow.org/api_docs/python/tf/raw_ops/Acosh

As Matt pointed out, for acos we should throw errors for bool, and for acosh we should throw errors for both bool and int

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille)


tfjs-backend-wasm/src/cc/kernels/Acosh.cc line 51 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

As Matt pointed out, for acos we should throw errors for bool, and for acosh we should throw errors for both bool and int

I think our current cpu backend, unary just convert the value to the tensor dtype directly using typed array, the behavior is bit different from TensorFlow implementation, for most part it is ok. Since WASM has better control on the conversion, I would prefer more align with TensorFlow. thanks

@chunnienc
Copy link
Collaborator Author

I removed the implementations for unexpected dtypes for these ops. It will dump an warning message in the console but not thrown an error. No framework-level changes in this PR. If we want to align the behavior more with TF, we may need some changes in tfjs-core for dtype checks (like what op cos has).

@chunnienc chunnienc requested review from mattsoulanille and pyu10055 and removed request for mattsoulanille and pyu10055 January 9, 2023 22:56
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille)

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: :shipit: complete! 2 of 1 approvals obtained

@chunnienc chunnienc merged commit ea8d7db into tensorflow:master Jan 11, 2023
@chunnienc chunnienc deleted the wasm-acos-acosh branch January 11, 2023 05:19
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.

3 participants