Add resource initializer support #6826

ahmedsabie · 2022-09-07T21:47:07Z

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

This change is

lina128

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

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 381 at r1 (raw file):

    for (input, resource_id) in model_input_to_resource_id.items():
      if input in signature_inputs:
        signature_inputs[input][common.RESOURCE_ID_KEY] = resource_id

Should inputs to initializer graph be part of signature_inputs, if so, will model look for these inputs during inference? If no needed, should we put these signature_inputs as initializer_signature_inputs and put in model_json[common.INITIALIZER_SIGNATURE_KEY]['inputs']?

Code quote:

signature_inputs

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 609 at r1 (raw file):

    return None

  model_resources_with_captured_input_index = []

The implementation is super specific to the design of model_resource, can you add some annotations about what this block of code is doing.

Code quote:

model_resources_with_captured_input_index = []

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 650 at r1 (raw file):

  resource_id_to_captured_input_index = {captured_input._id : i for i, captured_input in enumerate(concrete_func._captured_inputs)}
  captured_input_index_offset = len(concrete_func.inputs) - len(concrete_func._captured_inputs)

Again this code seem to be specific to the design of the concrete_func inputs and captured inputs, can you add some annotations?

Code quote:

  resource_id_to_captured_input_index = {captured_input._id : i for i, captured_input in enumerate(concrete_func._captured_inputs)}
  captured_input_index_offset = len(concrete_func.inputs) - len(concrete_func._captured_inputs)

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 806 at r1 (raw file):

  signature = _build_signature_def(
      frozen_graph, concrete_func.inputs, concrete_func.outputs, saved_model_sigature)

Does this mean resource inputs will be required for each model inference?

Code quote:

concrete_func.inputs

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py line 473 at r1 (raw file):

          'name': 'unknown:0',
          'dtype': 'DT_RESOURCE',
          'tensorShape': {},

Is it possible to not mix resource inputs from regular inputs?

Code quote:

        'unknown:0': {
          'name': 'unknown:0',
          'dtype': 'DT_RESOURCE',
          'tensorShape': {},
          'resourceId': None
        }

pyu10055

Thank you Ahmed for adding this critical feature.
One thing I am trying to get a sense, can you explain at high level, how is the DT_RESOURCE type tensor get de-referenced in the graph model executor?

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

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 609 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

The implementation is super specific to the design of model_resource, can you add some annotations about what this block of code is doing.

maybe link to TF code for the definition of model resources.

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 791 at r1 (raw file):

    if saved_model_dir:
      (frozen_graph,
       frozen_initializer_graph) = _freeze_saved_model_v1(saved_model_dir,

will initializer function in saved model v1 still be supported?

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 806 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Does this mean resource inputs will be required for each model inference?

looks like we can handle resource type of inputs now

tfjs-inference/src/file_handler.ts line 78 at r1 (raw file):

      }

      // TODO: Uncomment once table initializers are supported in TFJS.

is this because tfjs-inference is depending on new version of tfjs npm?

pyu10055

and it should be worthwhile to add an end2end test for tf v2 table initializer.

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

ahmedsabie

This PR doesn't change how DT_RESOURCE works, it is implemented already by treating a dummy scalar tensor as the index into a map that holds the real hash table. Basically all the hash table ops return this tensor and use it to index to the table.

added

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

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 381 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Should inputs to initializer graph be part of signature_inputs, if so, will model look for these inputs during inference? If no needed, should we put these signature_inputs as initializer_signature_inputs and put in model_json[common.INITIALIZER_SIGNATURE_KEY]['inputs']?

Initializer graph doesn't have any inputs, this code is assigning resource_ids to model inputs so they can be matched with initializer outputs, in Keras something similar is done except there is a reference from both graphs to the same resource

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 609 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

maybe link to TF code for the definition of model resources.

added

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 650 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Again this code seem to be specific to the design of the concrete_func inputs and captured inputs, can you add some annotations?

added

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 791 at r1 (raw file):

model_resources_with_captured_input_index
yes there are already tests for that

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 806 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

looks like we can handle resource type of inputs now

yes saved model graphs always take resources as inputs to the graph, but are auto passed in as captured inputs, so this approach is now closer to what is in saved model

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py line 473 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Is it possible to not mix resource inputs from regular inputs?

It would require making input handling code more complex since they essentially behave the same, except for resource inputs by default filled in by engine. Keeping it as regular inputs matches the TF implementation, and it shouldn't be a concern to the user because its not visible to them on the front end.

tfjs-inference/src/file_handler.ts line 78 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

is this because tfjs-inference is depending on new version of tfjs npm?

yes

lina128

Excellent job, thank you Ahmed!

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

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py line 473 at r1 (raw file):

Previously, ahmedsabie wrote…

It would require making input handling code more complex since they essentially behave the same, except for resource inputs by default filled in by engine. Keeping it as regular inputs matches the TF implementation, and it shouldn't be a concern to the user because its not visible to them on the front end.

Fair enough.

mattsoulanille · 2022-09-28T18:30:30Z

tfjs-converter/src/executor/test_data/hash_table_v2_model_loader.ts

@@ -0,0 +1,326 @@
+export const HASH_TABLE_MODEL_V2 = {


Suggested change

export const HASH_TABLE_MODEL_V2 = {

/**

* @license

* Copyright 2022 Google LLC.

* Licensed under the Apache License, Version 2.0 (the "License");

* you may not use this file except in compliance with the License.

* You may obtain a copy of the License at

*

* http://www.apache.org/licenses/LICENSE-2.0

*

* Unless required by applicable law or agreed to in writing, software

* distributed under the License is distributed on an "AS IS" BASIS,

* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

* See the License for the specific language governing permissions and

* limitations under the License.

* =============================================================================

*/

export const HASH_TABLE_MODEL_V2 = {

pyu10055

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

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 381 at r1 (raw file):

Previously, ahmedsabie wrote…

Initializer graph doesn't have any inputs, this code is assigning resource_ids to model inputs so they can be matched with initializer outputs, in Keras something similar is done except there is a reference from both graphs to the same resource

Help me to understand, why the resource ids need to be attached to inputs and initializer outputs?
How does the inference flow works in this case?
This seems to be critical to the who design, it would be great to add some comments to explain this part.

tfjs-converter/src/executor/test_data/hash_table_v2_model_loader.ts line 1 at r3 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

/**
 * @license
 * Copyright 2022 Google LLC.
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 * =============================================================================
 */

export const HASH_TABLE_MODEL_V2 = {

add the license header?

pyu10055

+100, thank you Ahmed!

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

ahmedsabie

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

tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py line 381 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Help me to understand, why the resource ids need to be attached to inputs and initializer outputs?
How does the inference flow works in this case?
This seems to be critical to the who design, it would be great to add some comments to explain this part.

added comment

tfjs-converter/src/executor/test_data/hash_table_v2_model_loader.ts line 1 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

add the license header?

Done.

This is causing regression e2e tests to fail: 1) saved_model_v1_with_hashtable. #REGRESSION convert_predict webgl {"WEBGL_VERSION":2,"WEBGL_CPU_FORWARD":false,"WEBGL_SIZE_UPLOAD_UNIFORM":0} Error: Arrays differ: actual[0] = -1, expected[0] = 3. To reproduce this, use node 16 in e2e/ and run `NIGHTLY=true ./scripts/test-ci.sh`, or, after running that to generate the required files, run `yarn karma start --tags '#REGRESSION'`. This reverts commit 42dee16.

ahmedsabie marked this pull request as draft September 7, 2022 21:47

ahmedsabie requested review from lina128 and pyu10055 and removed request for lina128 September 7, 2022 21:47

lina128 reviewed Sep 13, 2022

View reviewed changes

pyu10055 requested changes Sep 15, 2022

View reviewed changes

pyu10055 reviewed Sep 15, 2022

View reviewed changes

ahmedsabie force-pushed the resource-initializer branch from ae8aac7 to 2ccca3e Compare September 27, 2022 19:18

ahmedsabie commented Sep 27, 2022

View reviewed changes

ahmedsabie marked this pull request as ready for review September 27, 2022 19:19

ahmedsabie requested review from pyu10055 and lina128 September 27, 2022 19:19

lina128 approved these changes Sep 28, 2022

View reviewed changes

mattsoulanille reviewed Sep 28, 2022

View reviewed changes

ahmedsabie force-pushed the resource-initializer branch 2 times, most recently from 4bd9eff to e1438e5 Compare September 28, 2022 21:40

pyu10055 approved these changes Sep 30, 2022

View reviewed changes

pyu10055 reviewed Sep 30, 2022

View reviewed changes

ahmedsabie added 6 commits October 4, 2022 13:22

Add resource initializer support

d6ea249

Fix lint errors

dd1f025

Add clarifying comments

ebac12a

Fix test bug

8255013

Fix test

762beee

Add clarifying comment about resource ids

033e353

ahmedsabie force-pushed the resource-initializer branch from bf0f88d to 033e353 Compare October 4, 2022 17:22

ahmedsabie commented Oct 4, 2022

View reviewed changes

ahmedsabie merged commit 42dee16 into master Oct 4, 2022

ahmedsabie deleted the resource-initializer branch October 4, 2022 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource initializer support #6826

Add resource initializer support #6826

ahmedsabie commented Sep 7, 2022 •

edited by nsthorat

Loading

lina128 left a comment

pyu10055 left a comment

pyu10055 left a comment •

edited

Loading

ahmedsabie left a comment

lina128 left a comment

mattsoulanille Sep 28, 2022

pyu10055 left a comment

pyu10055 left a comment

ahmedsabie left a comment

Add resource initializer support #6826

Add resource initializer support #6826

Conversation

ahmedsabie commented Sep 7, 2022 • edited by nsthorat Loading

lina128 left a comment

Choose a reason for hiding this comment

pyu10055 left a comment

Choose a reason for hiding this comment

pyu10055 left a comment • edited Loading

Choose a reason for hiding this comment

ahmedsabie left a comment

Choose a reason for hiding this comment

lina128 left a comment

Choose a reason for hiding this comment

mattsoulanille Sep 28, 2022

Choose a reason for hiding this comment

pyu10055 left a comment

Choose a reason for hiding this comment

pyu10055 left a comment

Choose a reason for hiding this comment

ahmedsabie left a comment

Choose a reason for hiding this comment

ahmedsabie commented Sep 7, 2022 •

edited by nsthorat

Loading

pyu10055 left a comment •

edited

Loading