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.

Sign up for GitHub

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

Jump to bottom

Add qint8 and qint16 support for FillOp #41421

Merged
merged 3 commits into from Jul 27, 2020
Merged

Add qint8 and qint16 support for FillOp #41421

merged 3 commits into from Jul 27, 2020

Conversation

yongtang
Copy link
Member

This PR tries to address the issue raised in #26069 (comment) where
qint8 and qint16 were not supported for FillOp.

This PR add qint8 and qint16 support for FillOp.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Jul 15, 2020
@yongtang yongtang mentioned this pull request Jul 15, 2020
Closed
@yongtang yongtang added the kokoro:force-run Tests on submitted change label Jul 15, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 15, 2020
mihaimaruseac
mihaimaruseac previously approved these changes Jul 15, 2020
View reviewed changes
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 15, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 15, 2020
@gbaned gbaned self-assigned this Jul 16, 2020
@gbaned gbaned added comp:core issues related to core part of tensorflow prtype:bugfix PR to fix a bug labels Jul 16, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Jul 16, 2020
@mihaimaruseac
Copy link
Collaborator

Can you make the test also test in eager mode?

Also, we are seeing the following error, both in graph mode and in eager mode (removing the session related parts of the test):

Traceback (most recent call last):
  File ".../tensorflow/python/framework/tensor_util.py", line 331, in _AssertCompatible
    fn(values)
  File ".../tensorflow/python/framework/tensor_util.py", line 257, in _check_quantized
    _check_failed(values)
  File ".../tensorflow/python/framework/tensor_util.py", line 251, in _check_failed
    raise ValueError(v)
ValueError: 0

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<embedded stdlib>/unittest/case.py", line 59, in testPartExecutor
    yield
  File "<embedded stdlib>/unittest/case.py", line 605, in run
    testMethod()
  File ".../tensorflow/python/kernel_tests/constant_op_test.py", line 463, in testQintDtype
    z = array_ops.zeros([2, 3], dtype=dtype)
  File ".../tensorflow/python/util/dispatch.py", line 201, in wrapper
    return target(*args, **kwargs)
  File ".../tensorflow/python/ops/array_ops.py", line 2769, in wrapped
    tensor = fun(*args, **kwargs)
  File ".../tensorflow/python/ops/array_ops.py", line 2828, in zeros
    output = fill(shape, constant(zero, dtype=dtype), name=name)
  File ".../tensorflow/python/framework/constant_op.py", line 266, in constant
    allow_broadcast=True)
  File ".../tensorflow/python/framework/constant_op.py", line 284, in _constant_impl
    allow_broadcast=allow_broadcast))
  File ".../tensorflow/python/framework/tensor_util.py", line 458, in make_tensor_proto
    _AssertCompatible(values, dtype)
  File ".../tensorflow/python/framework/tensor_util.py", line 338, in _AssertCompatible
    (dtype.name, repr(mismatch), type(mismatch).__name__))
TypeError: Expected qint8, got 0 of type 'int' instead.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Jul 16, 2020
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Jul 16, 2020
@yongtang
Copy link
Member Author

Thanks @mihaimaruseac , the PR has been updated with session part removed. Also, I casts the qint types to int32 type before comparing it with numpy. Think this will resolve the error raised. Please take a look and let me know if the issue still persists.

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jul 17, 2020
mihaimaruseac
mihaimaruseac previously approved these changes Jul 17, 2020
View reviewed changes
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 17, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 17, 2020
@mihaimaruseac
Copy link
Collaborator

This still fails internally

Traceback (most recent call last):
  File ".../tensorflow/python/framework/tensor_util.py", line 331, in _AssertCompatible
    fn(values)
  File ".../tensorflow/python/framework/tensor_util.py", line 257, in _check_quantized
    _check_failed(values)
  File ".../tensorflow/python/framework/tensor_util.py", line 251, in _check_failed
    raise ValueError(v)
ValueError: 0

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<embedded stdlib>/unittest/case.py", line 59, in testPartExecutor
    yield
  File "<embedded stdlib>/unittest/case.py", line 605, in run
    testMethod()
  File ".../tensorflow/python/kernel_tests/constant_op_test.py", line 463, in testQintDtype
    z = array_ops.zeros([2, 3], dtype=dtype)
  File ".../tensorflow/python/util/dispatch.py", line 201, in wrapper
    return target(*args, **kwargs)
  File ".../tensorflow/python/ops/array_ops.py", line 2769, in wrapped
    tensor = fun(*args, **kwargs)
  File ".../tensorflow/python/ops/array_ops.py", line 2828, in zeros
    output = fill(shape, constant(zero, dtype=dtype), name=name)
  File ".../tensorflow/python/framework/constant_op.py", line 266, in constant
    allow_broadcast=True)
  File ".../tensorflow/python/framework/constant_op.py", line 284, in _constant_impl
    allow_broadcast=allow_broadcast))
  File ".../tensorflow/python/framework/tensor_util.py", line 458, in make_tensor_proto
    _AssertCompatible(values, dtype)
  File ".../tensorflow/python/framework/tensor_util.py", line 338, in _AssertCompatible
    (dtype.name, repr(mismatch), type(mismatch).__name__))
TypeError: Expected qint8, got 0 of type 'int' instead.

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed ready to pull PR ready for merge process labels Jul 20, 2020
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Jul 22, 2020
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Jul 22, 2020
@yongtang
Copy link
Member Author

Thanks @mihaimaruseac for the help. I think this is likely caused by dtype being reused (or graph being reused) in the internal test. I have split the tests into two to avoid this. The PR has been updated, I assume it will fix the error.

Please give another try and sorry for taking that long.

@yongtang yongtang added the kokoro:force-run Tests on submitted change label Jul 22, 2020
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jul 22, 2020
mihaimaruseac
mihaimaruseac previously approved these changes Jul 22, 2020
View reviewed changes
@google-ml-butler google-ml-butler bot added the ready to pull PR ready for merge process label Jul 22, 2020
@mihaimaruseac
Copy link
Collaborator

Thank you for the prompt response and sorry that the mismatch between internal and open source tests makes this harder to merge.

@mihaimaruseac
Copy link
Collaborator

//tensorflow/python/kernel_tests:constant_op_test still fails :(

Traceback (most recent call last):
  File "<embedded stdlib>/unittest/case.py", line 59, in testPartExecutor
    yield
  File "<embedded stdlib>/unittest/case.py", line 605, in run
    testMethod()
  File ".../tensorflow/python/kernel_tests/constant_op_test.py", line 471, in testQint16Dtype
    z = array_ops.zeros([2, 3], dtype=dtype)
  File ".../tensorflow/python/util/dispatch.py", line 201, in wrapper
    return target(*args, **kwargs)
  File ".../tensorflow/python/ops/array_ops.py", line 2769, in wrapped
    tensor = fun(*args, **kwargs)
  File ".../tensorflow/python/ops/array_ops.py", line 2830, in zeros
    output = fill(shape, constant(zero, dtype=dtype), name=name)
  File ".../tensorflow/python/framework/constant_op.py", line 266, in constant
    allow_broadcast=True)
  File ".../tensorflow/python/framework/constant_op.py", line 284, in _constant_impl
    allow_broadcast=allow_broadcast))
  File ".../tensorflow/python/framework/tensor_util.py", line 565, in make_tensor_proto
    append_fn(tensor_proto, proto_values)
  File ".../tensorflow/python/framework/fast_tensor_util.pyx", line 97, in fast_tensor_util.AppendInt8ArrayToTensorProto
ValueError: Buffer dtype mismatch, expected 'int8_t' but got 'short'

@gbaned gbaned removed kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 23, 2020
@yongtang yongtang mentioned this pull request Jul 23, 2020
Merged
@yongtang
Copy link
Member Author

Thanks @mihaimaruseac. I think the error is caused by the incorrect mapping of fast tensor <=> np which appear to be a bug unrelated. I have created a PR #41677 to address it.

@yongtang
Add qint8 and qint16 support for FillOp
f8c8a77 
This PR tries to address the issue raised in 26069 where
qint8 and qint16 were not supported for FillOp.

This PR add qint8 and qint16 support for FillOp.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Use np.zeros([]).astype(dtype.as_numpy_dtype) to specify qint8 zero
11c1cd9 
```
+    elif dtype.is_quantized:
+      zero = np.zeros([]).astype(dtype.as_numpy_dtype)
```

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Split test into two, as otherwise the same dtype will be reused in in…
52e516c 
…ternal tests

and cause testing to fail.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Jul 25, 2020
@yongtang
Copy link
Member Author

@mihaimaruseac With PR #41677 merged in, I have rebased and updated this PR. I think the internal test issue likely have been solved. Can you give it a try? Thanks a lot for the help in the process.

@yongtang yongtang removed the stat:awaiting response Status - Awaiting response from author label Jul 25, 2020
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jul 25, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 25, 2020
@kokoro-team kokoro-team removed kokoro:force-run Tests on submitted change labels Jul 25, 2020
@tensorflow-copybara tensorflow-copybara merged commit deaf738 into tensorflow:master Jul 27, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Jul 27, 2020
@yongtang yongtang deleted the 26069-fill-qint8-qint16 branch July 27, 2020 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihaimaruseac mihaimaruseac mihaimaruseac approved these changes

Assignees

@gbaned gbaned

Labels
cla: yes comp:core issues related to core part of tensorflow prtype:bugfix PR to fix a bug ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@yongtang @mihaimaruseac @gbaned @googlebot @kokoro-team @tensorflow-copybara @goldiegadde