Add Support for various TensorList ops

[pyu10055]

These TensorList ops and Stateful While ops are needed to for Keras GRU layers.

Here is the list of TensorList ops in TensorFlow
https://www.tensorflow.org/api_docs/python/tf/raw_ops/TensorListConcat?hl=vi

Added integration test that creates and validates a keras model with GRU layer.

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

---

This change is 

[lina128]

Reviewed 6 of 33 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

---

tfjs-converter/src/executor/execution_context.ts, line 48 at r1 (raw file):

  constructor(
      public readonly weightMap: NamedTensorsMap,

Why do we need to pass these four arguments? They don't seem to be used in the constructor. Or is it some advanced usage of class that implicitly set members?

---

tfjs-converter/src/executor/graph_executor.ts, line 301 at r1 (raw file):

      this.checkInputs(inputs);
      this.checkInputShapeAndType(inputs);
      outputs = this.mapOutputs(outputs);

This transformation is likely needed irrelevant of ignoreIOCheck flag, or is it just for checkOutputs?

[pyu10055]

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

---

tfjs-converter/src/executor/execution_context.ts, line 48 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Why do we need to pass these four arguments? They don't seem to be used in the constructor. Or is it some advanced usage of class that implicitly set members?

when add public qualifier, it becomes a public accessible class variable.

---

tfjs-converter/src/executor/graph_executor.ts, line 301 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

This transformation is likely needed irrelevant of ignoreIOCheck flag, or is it just for checkOutputs?

I will rename to ignoreIOCheckAndMapping, since mapping is not needed either.

[lina128]

Hi Ping, this is a pretty large PR, I think I finally understand the design. And I agree with the design. I left some nit comments, can be addressed in this PR or future PR. Thank you!

Reviewed 21 of 33 files at r1, 1 of 6 files at r2, 1 of 5 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @pyu10055)

---

tfjs-converter/src/executor/execution_context.ts, line 48 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

when add public qualifier, it becomes a public accessible class variable.

nit: If you make it readonly, you can drop the public qualifier since "TypeScript symbols are public by default. Never use the public modifier except when declaring non-readonly public parameter properties (in constructors)." (from http://go/tsstyle#visibility)

---

tfjs-converter/src/executor/execution_context.ts, line 49 at r2 (raw file):

  constructor(
      public readonly weightMap: NamedTensorsMap,
      public readonly tensorArrayMap: TensorArrayMap,

Can this be optional?

---

tfjs-converter/src/executor/execution_context.ts, line 50 at r2 (raw file):

      public readonly weightMap: NamedTensorsMap,
      public readonly tensorArrayMap: TensorArrayMap,
      public readonly tensorListMap: TensorListMap,

Can this be optional?

---

tfjs-converter/src/executor/graph_executor.ts, line 293 at r2 (raw file):

   * @param disableWarning disable the no dynamic ops warning message, default
   * to false
   * @param tensorArrayMap glboal TensorArray map by id.

@param tensorArrayMap global TensorArray map by id. Used for function execution.

---

tfjs-converter/src/executor/graph_executor.ts, line 294 at r2 (raw file):

   * to false
   * @param tensorArrayMap glboal TensorArray map by id.
   * @param tensorListMap glboal TensorList map by id.

@param tensorListMap global TensorList map by id. Used for function execution.

---

tfjs-converter/src/executor/graph_executor.ts, line 295 at r2 (raw file):

   * @param tensorArrayMap glboal TensorArray map by id.
   * @param tensorListMap glboal TensorList map by id.
   * @param ignoreIOCheckAndMapping ignore IO check and name mapping for

nit: Actually if this flag should only set true when it is function execution, then I think we should not expose this low level flag because the state can be computed from a high level flag isFunctionExecution. This way, it hides implementation details. Also we can use the isFunctionExecution flag to validate whether other parameters that are only supposed to be set for function execution (e.g. tensorArrayMap, etc.). And throw out warnings if they are used in other ways, this can prevent our developers from using the API in unexpected ways.

---

tfjs-converter/src/executor/graph_executor.ts, line 344 at r2 (raw file):

  async executeFunctionAsync(
      inputs: Tensor[], tensorArrayMap: TensorArrayMap,

Should these be optional?

---

tfjs-converter/src/executor/tensor_array.ts, line 54 at r2 (raw file):

  /**
   * Close the current TensorArray.

Dispose the tensors and idTensor and mark the TensoryArray as closed.

---

tfjs-converter/src/executor/tensor_array.ts, line 139 at r2 (raw file):

            index}.`);

    if (t && t.read) {

nit: This can be just if(t.read) right, because t has a safe guard, so it always exists.

---

tfjs-converter/src/executor/tensor_list.ts, line 67 at r2 (raw file):

  /**
   * Clean the current TensorList.

Dispose tensors and idTensors.

---

tfjs-converter/src/executor/tensor_list.ts, line 374 at r2 (raw file):

      const sliced = slice(tensor, indices, sizes);
      tensors[i] = sliced.reshape(elementShape);
      sliced.dispose();

This is in tidy, why need to explicit dispose?

---

tfjs-converter/src/operations/executors/control_executor.ts, line 83 at r2 (raw file):

        });

        const condResult =

This line of code seems to be a duplicate of above result = await ... code. I'm confused about the logic. Why executeFunctionAsync twice in a while loop?

---

tfjs-converter/src/operations/executors/control_executor.ts, line 328 at r2 (raw file):

      const tensorList = context.getTensorList(id);
      tensorList.pushBack(writeTensor);
      return [scalar(tensorList.id)];

Why not tensorList.idTensor?

---

tfjs-converter/src/operations/executors/control_executor_test.ts, line 660 at r2 (raw file):

    });
  });
  describe('TensorListReserve', () => {

I'm not sure for tests in this file, should we add memory tests?

[lina128]

Amazing work!

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

[pyu10055]

sorry, it got blown up while I am trying to fix the mem leak.

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

---

tfjs-converter/src/executor/execution_context.ts, line 48 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

nit: If you make it readonly, you can drop the public qualifier since "TypeScript symbols are public by default. Never use the public modifier except when declaring non-readonly public parameter properties (in constructors)." (from http://go/tsstyle#visibility)

THank you!

---

tfjs-converter/src/executor/execution_context.ts, line 49 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Can this be optional?

Done.

---

tfjs-converter/src/executor/execution_context.ts, line 50 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Can this be optional?

Done.

---

tfjs-converter/src/executor/graph_executor.ts, line 293 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

@param tensorArrayMap global TensorArray map by id. Used for function execution.

Done.

---

tfjs-converter/src/executor/graph_executor.ts, line 294 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

@param tensorListMap global TensorList map by id. Used for function execution.

Done.

---

tfjs-converter/src/executor/graph_executor.ts, line 295 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

nit: Actually if this flag should only set true when it is function execution, then I think we should not expose this low level flag because the state can be computed from a high level flag isFunctionExecution. This way, it hides implementation details. Also we can use the isFunctionExecution flag to validate whether other parameters that are only supposed to be set for function execution (e.g. tensorArrayMap, etc.). And throw out warnings if they are used in other ways, this can prevent our developers from using the API in unexpected ways.

great idea, created a private method to hide these params, and left the executeAsync method the same as before.

---

tfjs-converter/src/executor/graph_executor.ts, line 344 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Should these be optional?

Done.

---

tfjs-converter/src/executor/tensor_array.ts, line 54 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Dispose the tensors and idTensor and mark the TensoryArray as closed.

Done.

---

tfjs-converter/src/executor/tensor_list.ts, line 67 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Dispose tensors and idTensors.

Done.

---

tfjs-converter/src/executor/tensor_list.ts, line 374 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

This is in tidy, why need to explicit dispose?

Done.

---

tfjs-converter/src/operations/executors/control_executor.ts, line 83 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

This line of code seems to be a duplicate of above result = await ... code. I'm confused about the logic. Why executeFunctionAsync twice in a while loop?

this to get the condition of the loop not the loop body.

---

tfjs-converter/src/operations/executors/control_executor.ts, line 328 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Why not tensorList.idTensor?

good catch

---

tfjs-converter/src/operations/executors/control_executor_test.ts, line 660 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

I'm not sure for tests in this file, should we add memory tests?

no, this is all mocked, not really calling the backend to create tensors.

[lina128]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

---

tfjs-converter/src/operations/executors/control_executor.ts, line 83 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

this to get the condition of the loop not the loop body.

Got it, this is not intuitive from reading the code. Can we add some annotations?

[pyu10055]

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

---

tfjs-converter/src/operations/executors/control_executor.ts, line 83 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Got it, this is not intuitive from reading the code. Can we add some annotations?

Done. Thanks!

[lina128]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

---

tfjs-converter/src/executor/execution_context_test.ts, line 25 at r4 (raw file):

describe('ExecutionContext', () => {
  beforeEach(() => {
    context = new ExecutionContext({}, {}, {});

This can now just be ExecutionContext(), right? Much simpler! This can be a separate PR to change the reference style in all places in one pass.

---

tfjs-converter/src/executor/graph_executor.ts, line 344 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Done.

tensorArraypMap?: TensorArrayMap, tensorListMap?: TensorListMap

---

tfjs-converter/src/executor/graph_executor.ts, line 312 at r4 (raw file):

   * function execution.
   */
  private async _executeAsync(

I like this wrapper method, which leaves the high level api much cleaner, thank you Ping!

[lina128]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

---

tfjs-converter/src/operations/executors/control_executor.ts, line 83 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Done. Thanks!

The annotation helps a lot. I can just read through the annotation and skip the implementation details. This is great! Thank you Ping!

---

tfjs-converter/src/operations/executors/control_executor.ts, line 78 at r5 (raw file):

      while (condValue[0]) {
        // Record the previous result for intermediate tensor tracking
        const origResult = result;

Is prevResult a better name?

[lina128]

LGTM!