Add Kernel SparseFillEmptyRows for CPU and WebGL backend

[ahmedsabie]

ref #4838
TensorFlow python version is defined here
c++ kernel definition is here

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

---

This change is 

[pyu10055]

Thank you Ahmed, congrats on the first PR.
Please add reference for original issue in the PR description.
The PR looks great, just couple minor comments.

Reviewable status: 0 of 1 approvals obtained (waiting on @ahmedsabie and @lina128)

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 24 at r1 (raw file):

    values: number[], denseShape: number[], defaultValue: number):
    [TypedArray, number[], number[], boolean[], number[]] {
  const N = indicesShape[0];

in typescript, variable starts with lowercase, you can call it indicesCount

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 39 at r1 (raw file):

    const outputIndices =
        util.getArrayFromDType(indicesDType, 0 * rank) as TypedArray;
    const outputValues: number[] = [];

You should treat the outputValue the same way as outputIndices, create a TypeArray with the same dtype as inputValue, instead of forcing it as Int32Array.

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 51 at r1 (raw file):

  for (let i = 0; i < N; ++i) {
    // indices is a 2d tensor with shape of [N, rank]
    const row = indices[i * rank + 0];

no need to add 0 here.

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 101 at r1 (raw file):

    for (let i = 0; i < N; ++i) {
      // indices is a 2d tensor with shape of [N, rank]
      const row = indices[i * rank + 0];

• 0 is not needed

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 122 at r1 (raw file):

        // Just need to set the row index in the right location.
        // outputIndices is a 2d tensor with shape of [N, rank]

remove the empty line here

---

tfjs-core/src/kernel_names.ts, line 773 at r1 (raw file):

    Pick<NamedTensorInfoMap, 'indices'|'values'|'denseShape'>;
export interface SparseFillEmptyRowsAttrs {
  defaultValue: number;

defaultValue should also be part of the input. https://github.com/tensorflow/tensorflow/blob/v2.4.1/tensorflow/core/ops/sparse_ops.cc#L598

---

tfjs-core/src/ops/sparse/sparse_fill_empty_rows.ts, line 72 at r1 (raw file):

 *     0] for rows missing from the input sparse tensor. output indices: 2-D.
 *     the indices of the filled sparse tensor.

remove the output indices part, since it has been explained in the return section

---

tfjs-core/src/ops/sparse/sparse_fill_empty_rows.ts, line 108 at r1 (raw file):

    denseShape: $denseShape
  };
  const attrs: SparseFillEmptyRowsAttrs = {defaultValue};

move the defaultValue to inputs interface to be more aligned with TF definition.

[lina128]

Great job Ahmed! And congratulations on your first PR!
Can you update the PR title to Add Kernel SparseFillEmptyRowsConfig for CPU and WebGL backend.

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

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows.ts, line 47 at r1 (raw file):

  const $indices = backend.data.get(indices.dataId).values as TypedArray;
  const $values =
      Array.from(backend.data.get(values.dataId).values as TypedArray);

Do you need Array.from?

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 39 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

You should treat the outputValue the same way as outputIndices, create a TypeArray with the same dtype as inputValue, instead of forcing it as Int32Array.

+1

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 95 at r1 (raw file):

    const outputIndices =
        util.getArrayFromDType(indicesDType, nFull * rank) as TypedArray;
    const outputValues: number[] = new Array(nFull);

Same as above, use TypedArray.

---

tfjs-backend-webgl/src/kernels/SparseFillEmptyRows.ts, line 46 at r1 (raw file):

  const $indices = backend.readSync(indices.dataId).values as TypedArray;
  const $values =
      Array.from(backend.readSync(values.dataId).values as TypedArray);

No need to use Array.from for values.

---

tfjs-core/src/kernel_names.ts, line 773 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

defaultValue should also be part of the input. https://github.com/tensorflow/tensorflow/blob/v2.4.1/tensorflow/core/ops/sparse_ops.cc#L598

If it's not a tensor, shouldn't it be attr? See nonMaxSuppression for example: https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/kernel_names.ts#L598

I think engine expect inputs to be NamedTensorMap

---

tfjs-core/src/ops/sparse/sparse_fill_empty_rows.ts, line 108 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

move the defaultValue to inputs interface to be more aligned with TF definition.

If defaultValue is not tensor, it should be attribute, isn't that our convention?

[pyu10055]

Reviewable status: 0 of 1 approvals obtained (waiting on @ahmedsabie, @lina128, and @pyu10055)

---

tfjs-core/src/ops/sparse/sparse_fill_empty_rows.ts, line 108 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

If defaultValue is not tensor, it should be attribute, isn't that our convention?

The defaultValue is part of the inputs (a scalar tensor) in TF definition, when we make it into number in the API, it would force graph executor to call readSync, which is the main problem for WebGPU issues.
If it is attribute in the Graph, then the value won't need readSync.

[lina128]

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

---

tfjs-core/src/ops/sparse/sparse_fill_empty_rows.ts, line 108 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

The defaultValue is part of the inputs (a scalar tensor) in TF definition, when we make it into number in the API, it would force graph executor to call readSync, which is the main problem for WebGPU issues.
If it is attribute in the Graph, then the value won't need readSync.

Changing the convention in this single op doesn't help solve that problem. Do we have a plan to fix this issue for all impacted ops?

[pyu10055]

Reviewable status: 0 of 1 approvals obtained (waiting on @ahmedsabie and @lina128)

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 24 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

in typescript, variable starts with lowercase, you can call it indicesCount

thanks

---

tfjs-backend-cpu/src/kernels/SparseFillEmptyRows_impl.ts, line 51 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

no need to add 0 here.

resolved

---

tfjs-core/src/kernel_names.ts, line 773 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

If it's not a tensor, shouldn't it be attr? See nonMaxSuppression for example: https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/kernel_names.ts#L598

I think engine expect inputs to be NamedTensorMap

any inputs to the node is a tensor in the TF kernel definition. In this case the default value is a scalar tensor.

---

tfjs-core/src/ops/sparse/sparse_fill_empty_rows.ts, line 108 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Changing the convention in this single op doesn't help solve that problem. Do we have a plan to fix this issue for all impacted ops?

I believe one of the modularization goals is to tie our kernels to the TF op definition. I think we will need to fix other op APIs that deviate from TF, we should allow all kernel inputs to be tensors. I know it is a large set of ops we need to fix, but I think it is the right direction.

[lina128]

Reviewable status: 0 of 1 approvals obtained (waiting on @ahmedsabie, @lina128, and @pyu10055)

---

tfjs-core/src/ops/sparse/sparse_fill_empty_rows.ts, line 108 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

I believe one of the modularization goals is to tie our kernels to the TF op definition. I think we will need to fix other op APIs that deviate from TF, we should allow all kernel inputs to be tensors. I know it is a large set of ops we need to fix, but I think it is the right direction.

Got it. Good to know we are going to fix this.

[pyu10055]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @ahmedsabie)

[lina128]

LGTM

Reviewable status: complete! 2 of 1 approvals obtained (waiting on @ahmedsabie)