Skip to content

Conversation

@pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented May 30, 2020

Allow complex64 weight from saved model to be encoded into tfjs binary files.
Add serialize/deserialize capability to js weight loader.
Add integration test for e2e conversion and inference.
Updated the e2e infra to use the latest python code instead of released pip package.

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


This change is Reviewable

@pyu10055 pyu10055 requested a review from lina128 May 30, 2020 18:51
Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

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


e2e/integration_tests/convert_predict.ts, line 65 at r1 (raw file):

          await tfc.setBackend(backend);

          console.log(model);

Remove this line?


e2e/integration_tests/convert_predict.ts, line 69 at r1 (raw file):

              `${KARMA_SERVER}/${DATA_URL}/${model}/model.json`);

          const xs = createInputTensors(inputsData, inputsShapes);

The dtype information is not used here. Does it matter? I think it defaults to float32.


e2e/integration_tests/requirements-dev.txt, line 2 at r1 (raw file):

keras==2.3.1
-r ../../tfjs-converter/python/requirements.txt

Why not requirements-dev.txt?


e2e/scripts/setup-py-env.sh, line 65 at r1 (raw file):

  pip3 install -r requirements-dev.txt
fi

Can you add an annotation or echo here?


tfjs-converter/python/requirements.txt, line 4 at r1 (raw file):

numpy>=1.16.4
six>=1.12.0
tensorflow-cpu==2.1.0

If we change to pinned version, will it have conflict with tfx on their side?


tfjs-converter/python/tensorflowjs/write_weights.py, line 371 at r1 (raw file):

  if not (data.dtype in _OUTPUT_DTYPES or data.dtype in _AUTO_DTYPE_CONVERSION):
    print(_OUTPUT_DTYPES)

Remove this line?


tfjs-core/src/io/io_utils.ts, line 18 at r1 (raw file):

 */

import * as tf from '../index';

This imports everything. Can we import the only the part needed?


tfjs-core/src/io/io_utils.ts, line 180 at r1 (raw file):

        const real = new Float32Array(values.length / 2);
        const image = new Float32Array(values.length / 2);
        for (let i = 0; i < values.length / 2; i++) {

Curious, will values.length / 2 be computed each round?

Copy link
Collaborator Author

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

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


e2e/integration_tests/convert_predict.ts, line 65 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Remove this line?

Done.


e2e/integration_tests/convert_predict.ts, line 69 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

The dtype information is not used here. Does it matter? I think it defaults to float32.

yes, I will figure out how to utilize them in a following PR.


e2e/integration_tests/requirements-dev.txt, line 2 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Why not requirements-dev.txt?

the dev requirement includes pylint which is not need for intergration tests.


e2e/scripts/setup-py-env.sh, line 65 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Can you add an annotation or echo here?

Done.


tfjs-converter/python/requirements.txt, line 4 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

If we change to pinned version, will it have conflict with tfx on their side?

I filed a bug against keras, there is a breakage on our json keras loader in TF 2.2.0. We will update this once that is resolved.
tfx does not use that, they should be fine.


tfjs-converter/python/tensorflowjs/write_weights.py, line 371 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Remove this line?

Done.


tfjs-core/src/io/io_utils.ts, line 180 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Curious, will values.length / 2 be computed each round?

good point, updated.

Copy link
Collaborator Author

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

thanks for the review, PTAL.

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


tfjs-core/src/io/io_utils.ts, line 18 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

This imports everything. Can we import the only the part needed?

Done.

Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Thank you, Ping! LGTM!

Reviewed 6 of 16 files at r1, 2 of 4 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)


e2e/integration_tests/convert_predict.ts, line 69 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

yes, I will figure out how to utilize them in a following PR.

You can save the dtype in a file, same as how other inputs are saved. the file can follow the name pattern ${model}.xs-dtype.json

@pyu10055 pyu10055 merged commit 52e27ca into master Jun 1, 2020
@pyu10055 pyu10055 deleted the complex_weights branch June 1, 2020 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants