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

[oneDNN] lookup_table op with support for BF16 data type. #31558

Merged
merged 20 commits into from
Mar 19, 2021

Conversation

arogowie-intel
Copy link
Contributor

@arogowie-intel arogowie-intel commented Mar 11, 2021

PR types

New features

PR changes

OPs

Describe

This PR adds support for BF16 data type in lookup_table op for both forward and backward pass.

@arogowie-intel
Copy link
Contributor Author

@jczaja @wozna @wojtuss @arlesniak @lidanqing-intel Please start your review.

Copy link
Contributor

@wozna wozna left a comment

Choose a reason for hiding this comment

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

Well done python tests :)
LGTM

@@ -102,7 +102,8 @@ class LookupTableKernel : public framework::OpKernel<T> {
auto id_index = table_t.GetIndexFromId(ids[i]);

if (id_index != -1) {
if (input_data_type == framework::proto::VarType::INT8) {
if (input_data_type == framework::proto::VarType::INT8 ||
input_data_type == framework::proto::VarType::BF16) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why do you change lookup_table_op but not lookup_table_v2_op?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because in the model "word2vec" there is lookup_table op used. Moreover paddle.fluid.layers.embedding Py API creates lookup_table op.

@@ -33,10 +33,19 @@
from paddle.fluid.op import Operator
from paddle.fluid.executor import Executor
from paddle.fluid.framework import Program, OpProtoHolder, Variable
from testsuite import create_op, set_input, append_input_output, append_loss_ops
from paddle.fluid.tests.unittests.testsuite import (
Copy link
Contributor

Choose a reason for hiding this comment

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

why change this file?

Copy link
Contributor Author

@arogowie-intel arogowie-intel Mar 16, 2021

Choose a reason for hiding this comment

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

Since this is the recommended Python style for importing multiple modules from single package PEP328 and generally it looks more readable. Moreover this way you can have multiline import statement.

@arogowie-intel
Copy link
Contributor Author

@luotao1 regarding issues rised in PR-CI-APPROVAL:

  1. @unittest.skipIf(not core.supports_bfloat16(),

    I need to use this decorator, since otherwise the CI configurations with GPU will throw errors about no kernel registered for this datatype for GPU.

  2. | self.check_output_with_place(core.CPUPlace(), check_dygraph=False)

    The check_dygraph=False is needed to pass tests. The BF16 data type is not supported in dygraph mode.

  3. | max_relative_error=1.5e-2,

    Usage of BF16 data type might incur slightly lower accuracy in comparison to FP32.

  4. | @skip_check_grad_ci(

    This is analogous as in tests here - citing: "gradient of paddings makes no sense."

Copy link
Contributor

@arlesniak arlesniak left a comment

Choose a reason for hiding this comment

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

Good job!

@luotao1 luotao1 merged commit a4a2b77 into PaddlePaddle:develop Mar 19, 2021
@arogowie-intel arogowie-intel deleted the aosewski/bf16_lookup branch March 19, 2021 08:44
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.

4 participants