Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Mutablehashtable lookup support full size dynamic default values. #43269

Merged
merged 1 commit into from Oct 27, 2020
Merged

Mutablehashtable lookup support full size dynamic default values. #43269

merged 1 commit into from Oct 27, 2020
+124 −16

Conversation

rhdong
Copy link
Member

@rhdong rhdong commented Sep 16, 2020

This PR is one part of RFC: Sparse Domain Isolation for Supporting large-scale Sparse Weights Training, and original is here : PR
@yuefengz @tanzhenyu Please review the PR, Thanks!

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Sep 16, 2020
@gbaned gbaned self-assigned this Sep 17, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 17, 2020
@rhdong
Copy link
Member Author

rhdong commented Sep 21, 2020 •
edited

This PR is required to make Hash table trainable in TensorFlow, that is a key feature for sparse weights training on Recommender System.

@gbaned gbaned added the awaiting review Pull request awaiting review label Sep 23, 2020
@rhdong
Copy link
Member Author

rhdong commented Sep 29, 2020

Hi @rohan100jain @yuefengz , please help review this PR, Thanks!

rohan100jain
rohan100jain requested changes Oct 14, 2020
View reviewed changes
tensorflow/python/ops/lookup_ops.py Show resolved Hide resolved
tensorflow/python/ops/lookup_ops.py Show resolved Hide resolved
tensorflow/core/kernels/lookup_table_op.cc Outdated

tf_shared_lock l(mu_);
for (int64 i = 0; i < key_values.size(); ++i) {
value_values(i) = gtl::FindWithDefault(
table_, SubtleMustCopyIfIntegral(key_values(i)), default_val);
table_, SubtleMustCopyIfIntegral(key_values(i)),
is_full_default ? default_flat(i) : default_flat(0));
Copy link
Member

Choose a reason for hiding this comment

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

So if is_full_default == false, we expect default_total = 1 i.e. a single scalar?

Copy link
Member Author

@rhdong rhdong Oct 15, 2020 •
edited

Choose a reason for hiding this comment

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

So if is_full_default == false, we expect default_total = 1 i.e. a single scalar?

Yes, for backward compatibility, maybe you can pay attention to this change const auto default_flat = default_value.flat<V>();
The size of dynamic_default_values will be required to be equal to self._default_value.size() or keys.size() * self._default_value.size(). In the latter case, we set different default values for each keys.

Copy link
Member

Choose a reason for hiding this comment

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

Please add comments describing this logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Oct 14, 2020
@rhdong rhdong force-pushed the RFC237-patch-for-recsys-sig branch from cb293ec to 9343aea Compare October 16, 2020 12:23
@rhdong
Copy link
Member Author

rhdong commented Oct 16, 2020

Hi @rohan100jain , thank you for your reviewing, I just push a new commit that contains test cases, clearer function comments. Please review it again, thank you very much!

rohan100jain
rohan100jain requested changes Oct 16, 2020
View reviewed changes
tensorflow/python/ops/lookup_ops.py Outdated Show resolved Hide resolved
tensorflow/python/kernel_tests/lookup_ops_test.py Outdated Show resolved Hide resolved
tensorflow/core/kernels/lookup_table_op.cc Outdated

tf_shared_lock l(mu_);
for (int64 i = 0; i < key_values.size(); ++i) {
value_values(i) = gtl::FindWithDefault(
table_, SubtleMustCopyIfIntegral(key_values(i)), default_val);
table_, SubtleMustCopyIfIntegral(key_values(i)),
is_full_default ? default_flat(i) : default_flat(0));
Copy link
Member

Choose a reason for hiding this comment

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

Please add comments describing this logic.

tensorflow/python/kernel_tests/lookup_ops_test.py Outdated Show resolved Hide resolved
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 18, 2020
@rhdong rhdong force-pushed the RFC237-patch-for-recsys-sig branch from 9343aea to 29117ee Compare October 20, 2020 14:35
@gbaned gbaned requested review from rohan100jain and removed request for rohan100jain October 23, 2020 14:08
rohan100jain
rohan100jain approved these changes Oct 24, 2020
View reviewed changes
tensorflow/python/ops/lookup_ops.py Outdated Show resolved Hide resolved
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 24, 2020
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Oct 24, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 24, 2020
@rhdong rhdong force-pushed the RFC237-patch-for-recsys-sig branch from 29117ee to 3b11daf Compare October 26, 2020 11:06
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Oct 26, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 26, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 26, 2020
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Oct 27, 2020
@copybara-service copybara-service bot merged commit b0784e5 into tensorflow:master Oct 27, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Oct 27, 2020
rhdong added a commit to rhdong/tensorflow that referenced this pull request Dec 21, 2020
@rhdong
Merge pull request tensorflow#43269 from rhdong:Mutablehashtable look…
a26eed2 
…up support full size dynamic default values.

This PR is one part of RFC:tensorflow/community#237
@rhdong rhdong mentioned this pull request Dec 21, 2020
Merged
mihaimaruseac added a commit that referenced this pull request Jan 19, 2021
@mihaimaruseac
Merge pull request #45896 from rhdong/github/rhdong/r2.4
6d18335 
Merge pull request #43269 from rhdong:Mutablehashtable lookup support…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohan100jain rohan100jain rohan100jain approved these changes

@yuefengz yuefengz Awaiting requested review from yuefengz

Assignees

@gbaned gbaned

Labels
cla: yes ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rhdong @rohan100jain @googlebot @kokoro-team @tensorflowbutler @gbaned