-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add LookupTableSize and LookupTableSizeV2 ops #4755
Add LookupTableSize and LookupTableSizeV2 ops #4755
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
Reviewed 7 of 7 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @fneubaum , overall looks good, I left some minor comments.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @fneubaum-sulvo)
tfjs-converter/metadata/kernel2op.json, line 262 at r2 (raw file):
"LookupTableImportV2": [], "LookupTableSize": [ "scalar"
Remove 'scalar', just leave it blank. Because there's no tfjs op that maps to this kernel.
tfjs-converter/metadata/kernel2op.json, line 265 at r2 (raw file):
], "LookupTableSizeV2": [ "scalar"
Same.
tfjs-converter/python/tensorflowjs/op_list/hash_table.json, line 206 at r2 (raw file):
} ], "attrs": [
Remove the attrs field, basically match what is described in the TF op pbtxt file: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/ops/ops.pbtxt#L22817-L22828
tfjs-converter/python/tensorflowjs/op_list/hash_table.json, line 231 at r2 (raw file):
} ], "attrs": [
Remove attrs.
tfjs-converter/src/operations/executors/hash_table_executor.ts, line 77 at r2 (raw file):
const hashTable = resourceManager.getHashTableById(handle.id); return [await tfOps.scalar(hashTable.size())];
scalar(hashTable.size(), 'int32')
tfjs-converter/src/operations/executors/hash_table_executor_test.ts, line 490 at r2 (raw file):
const size = await result[0].data(); expect(size[0]).toEqual(0);
test_util.expectArraysClose(size, [0]);
tfjs-converter/src/operations/executors/hash_table_executor_test.ts, line 523 at r2 (raw file):
const size = await result[0].data(); expect(size[0]).toEqual(2);
test_util.expectArraysClose(size, [2]);
tfjs-converter/src/operations/executors/hash_table_executor_test.ts, line 558 at r2 (raw file):
const size = await result[0].data(); expect(size[0]).toEqual(0);
Same.
tfjs-converter/src/operations/executors/hash_table_executor_test.ts, line 591 at r2 (raw file):
const size = await result[0].data(); expect(size[0]).toEqual(2);
Same.
tfjs-converter/src/operations/op_list/hash_table.ts, line 124 at r2 (raw file):
{'start': 0, 'name': 'tableHandle', 'type': 'tensor'} ], 'attrs': [
Remove attrs.
tfjs-converter/src/operations/op_list/hash_table.ts, line 139 at r2 (raw file):
{'start': 0, 'name': 'tableHandle', 'type': 'tensor'} ], 'attrs': [
Remove attrs.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @lina128 ! You're very helpful.
I made the changes and now the code should be in better shape, I left a couple of questions but mostly to be sure the code is correct.
I need the following operations too as I have a model that depends on them: SparseSegmentMean, StringToHashBucketFast, SparseReshape, SparseFillEmptyRows. Sorry to ask but is there some help or pointers you could provide? Mainly for the Sparse ops as only SparseToDense is implemented. Thanks in advance!
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128)
tfjs-converter/metadata/kernel2op.json, line 262 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Remove 'scalar', just leave it blank. Because there's no tfjs op that maps to this kernel.
This was actually added by a script when I ran the tests, kernels_to_ops.ts . I had to do some refactoring adding a tensorSize method to the hashtable to make it work. Let me know if you think it's OK. Thanks!
tfjs-converter/metadata/kernel2op.json, line 265 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Same.
Same as before.
tfjs-converter/python/tensorflowjs/op_list/hash_table.json, line 206 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Remove the attrs field, basically match what is described in the TF op pbtxt file: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/ops/ops.pbtxt#L22817-L22828
Thank you! One question, in the file you linked there's both input and output arguments for these operations, but I can't find operations with the output argument set. Is it not used? Shall I omit it? Again, thanks for your time!
tfjs-converter/python/tensorflowjs/op_list/hash_table.json, line 231 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Remove attrs.
Done.
tfjs-converter/src/operations/executors/hash_table_executor.ts, line 77 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
scalar(hashTable.size(), 'int32')
I did this change, but as I moved it to the the method it's there.
tfjs-converter/src/operations/executors/hash_table_executor_test.ts, line 490 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
test_util.expectArraysClose(size, [0]);
Done.
tfjs-converter/src/operations/executors/hash_table_executor_test.ts, line 523 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
test_util.expectArraysClose(size, [2]);
Done.
tfjs-converter/src/operations/executors/hash_table_executor_test.ts, line 558 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Same.
Done.
tfjs-converter/src/operations/executors/hash_table_executor_test.ts, line 591 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Same.
Done.
tfjs-converter/src/operations/op_list/hash_table.ts, line 124 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Remove attrs.
Done, same as my other comment, do I need to set the output too? Thank you!
tfjs-converter/src/operations/op_list/hash_table.ts, line 139 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Remove attrs.
Done.
2f38f25
to
d2fd69a
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@googlebot I signed it!
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @lina128)
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Added LookupTableSize and LookupTableSizeV2 operations. What I do is get the size of the hash table and return it as a Rank 0 Tensor. The PR includes unit tests and also I tried it with the model I'm trying to convert, as described in this issue, LookupTableSizeV2 isn't shown as unsupported anymore.
This change is