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

[feat] Add HKV which is based on merlin HierarchicalHV #356

Closed
wants to merge 6 commits into from
Closed

[feat] Add HKV which is based on merlin HierarchicalHV #356

wants to merge 6 commits into from

Conversation

LinGeLin
Copy link
Contributor

@LinGeLin LinGeLin commented Aug 18, 2023 •
edited
Loading

Description

Added hkv hashtable which is based on HierarchicalKV. This hashtable can be used to hierarchical store key-value (feature-embedding) on high-bandwidth memory (HBM) of GPUs and in host memory.

Type of change

  • Bug fix
  • New Tutorial
  • Updated or additional documentation
  • Additional Testing
  • New Feature

Checklist:

  • I've properly formatted my code according to the guidelines
    • By running yapf
    • By running clang-format
  • This PR addresses an already submitted issue for TensorFlow Recommenders-Addons
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

How Has This Been Tested?

If you're adding a bugfix or new feature please describe the tests that you ran to verify your changes:
*

@LinGeLin LinGeLin requested a review from rhdong as a code owner August 18, 2023 03:09
@rhdong rhdong requested review from Lifann and MoFHeka August 18, 2023 03:29
rhdong
rhdong reviewed Sep 11, 2023
View reviewed changes
tensorflow_recommenders_addons/dynamic_embedding/core/kernels/hkv_hashtable_op_gpu.cu.cc Outdated
return Status::OK();
}

Status ExportValuesWithMetas(OpKernelContext* ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

We have changed the Metas concept to Scores.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @LinGeLin , It would be better to change all of the Meta to Score. In the HKV, we do have change it permarnently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I update it later? Because we are currently using HKV bate6, and the changes to scores are in bate7. The code for setting the thrust custom allocator has not been released yet, so I would like to update it later. Also, we haven't registered the ExportMate operator yet, so we can consider it as a feature update in the future. Is that okay?

Copy link
Member

Choose a reason for hiding this comment

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

If the user API is not named xxMetaxx, it's OK.

rhdong
rhdong reviewed Sep 22, 2023
View reviewed changes
tensorflow_recommenders_addons/dynamic_embedding/core/kernels/lookup_impl/lookup_table_op_hkv.h
default_allocator_ = std::make_unique<nv::merlin::DefaultAllocator>();
}

TFOrDefaultAllocator(OpKernelContext* ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

Hi @Lifann, would you please help reviewing this class? Thank you~

@LinGeLin LinGeLin force-pushed the hkv_opt branch 6 times, most recently from 15ea0fc to cdbd764 Compare November 6, 2023 13:39
@LinGeLin LinGeLin force-pushed the hkv_opt branch 4 times, most recently from 9d54997 to 045f5cb Compare November 15, 2023 03:14
@rhdong
Copy link
Member

rhdong commented Nov 21, 2023

Please add a proper prefix for the PR title and each commit, like [Feat], [Fix], and so on.

@LinGeLin LinGeLin changed the title Hkv opt [feat] Add HKV which is based on merlin HierarchicalHV Nov 22, 2023
@LinGeLin LinGeLin closed this Nov 22, 2023
@LinGeLin
Copy link
Contributor Author

Please add a proper prefix for the PR title and each commit, like [Feat], [Fix], and so on.

DONE

@LinGeLin LinGeLin reopened this Nov 22, 2023
@rhdong
Copy link
Member

rhdong commented Nov 22, 2023

The time consumption of CI is reduced significantly! Well done!

rhdong
rhdong reviewed Nov 26, 2023
View reviewed changes
tensorflow_recommenders_addons/dynamic_embedding/python/ops/hkv_hashtable_ops.py Outdated
values)
return op

def accum(self, keys, values_or_deltas, exists, name=None, shared_name=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could you add scores=None after the exists for reserving the customized strategy supporting?

rhdong
rhdong reviewed Nov 26, 2023
View reviewed changes
tensorflow_recommenders_addons/dynamic_embedding/python/ops/hkv_hashtable_ops.py Outdated
)
return (values, exists) if return_exists else values

def insert(self, keys, values, name=None, shared_name=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could you add scores=None after the values for reserving the customized strategy supporting?

@rhdong
Copy link
Member

rhdong commented Nov 26, 2023 •
edited
Loading

Hi @LinGeLin, I have no more comments. After your feedback on them, we can merge the PR. Thank you for your excellent job!

@rhdong rhdong self-requested a review November 29, 2023 19:39
@LinGeLin LinGeLin force-pushed the hkv_opt branch 3 times, most recently from 07b5ca2 to 87d6224 Compare December 20, 2023 03:19
rhdong
rhdong reviewed Dec 29, 2023
View reviewed changes
tensorflow_recommenders_addons/dynamic_embedding/core/kernels/hkv_hashtable_op_gpu.cu.cc
@@ -81,9 +83,17 @@ class HkvHashTableOfTensorsGpu final : public LookupInterface {
ctx, (max_hbm_for_vectors_i64 >= 0),
errors::InvalidArgument("params max_hbm_for_vectors less than 0"));

OP_REQUIRES_OK(ctx, GetNodeAttr(kernel->def(), "evict_global_epoch",
Copy link
Member

@rhdong rhdong Dec 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The evict_global_epoch is required only when the strategy is EpochLru or EpochLfu. So, it should be under a condition of if (Strategy == kEpochLru/Lfu).

You may need to refer to this logic of global epoch and its default value: https://github.com/NVIDIA-Merlin/HierarchicalKV/blob/master/include/merlin_hashtable.cuh#L2262-L2270

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 0. It doesn't matter, does it?

@LinGeLin LinGeLin closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhdong rhdong rhdong left review comments

@Lifann Lifann Awaiting requested review from Lifann

@MoFHeka MoFHeka Awaiting requested review from MoFHeka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LinGeLin @rhdong @Lifann