-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Added DLRM notebook and generated python script #2131
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
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR! Left some comments, let me know what you think :).
Here are some notes for posterity (we don't need to implement these now in this PR):
- We should, in the future, also think about the specific case of how how DLRM implements distributed training. The embeddings are sharded across devices, whereas the MLP layers are DDP'd.
- The paper mentions that they use
torch.nn.EmbeddingBag
instead oftorch.nn.Embedding
. Apparently,torch.nn.EmbeddingBag
is more efficient thantorch.nn.Embedding
followed by some aggregation op. We should think about this at some later point (maybe,keras_rs.embeddings.DistributedEmbedding
solves this). Anyway, we can worry about this later!
Updated docstrings to match the context according to code
@kharshith-k - let me know when this is ready for another round of review. Thanks! |
|
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.
Thanks! Great work, this looks good to me, overall. Just one major comment on using dense features (it isn't DLRM without the two blocks for dense and categorical features)
Delete duplicate dlrm.ipynb
Hi @abheesht17, The changes are ready for review. I've used raw_user_age and timestamp for continuous features. Please let me know if any more changes would be required. Thanks! |
Added DLRM.ipynb and generated dlrm.py