Conversation
Documentation preview |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -0,0 +1,364 @@ | |||
| { | |||
There was a problem hiding this comment.
why do we propose with Loader(train, batch_size=1024) as loader: which is different to our TensorFlow examples?
Reply via ReviewNB
There was a problem hiding this comment.
This is something @edknv suggested, it ensures that the background thread gets removed. I am working on a way to see if we can move this inside our model/trainer code. Because I think the context manager approach works for single GPU, but I don't think it will work in a multi-GPU setting.
| @@ -0,0 +1,364 @@ | |||
| { | |||
There was a problem hiding this comment.
I am not sure, if we have the next notebook available on PyTorch - we might need to reference the TensorFlow one OR the next steps are removed OR we link to the other training examples
Reply via ReviewNB
There was a problem hiding this comment.
good point! removed the cell for now and will add it once we have more examples
| "id": "23d9bf34", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "<img src=\"https://developer.download.nvidia.com/notebooks/dlsw-notebooks/merlin_models_01-getting-started/nvidia_logo.png\" style=\"width: 90px; float: right;\">\n", |
There was a problem hiding this comment.
we might need to update the logo?
There was a problem hiding this comment.
yes, absolutely! good point, created a new tracking logo
| @@ -0,0 +1,348 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #4. model.initialize(train_loader)
can we add some explanation why we do need model.initialize() step?
Reply via ReviewNB
There was a problem hiding this comment.
added note on the functionality of initialize
| @@ -0,0 +1,348 @@ | |||
| { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I am not sure -- was just copying over what we have on the TF side, we follow the same pattern there
* fix docstring * add test and cleanup * add example
radekosmulski
force-pushed
the
add_pytorch_DLRM_example
branch
from
7dfa115 to
7368970
Compare
4 participants
This adds the first pytorch example 🥳
Additionally, I propose in
DLRMModelwe renamedimtoembedding_dim. This aligns us with the tf api and (which is probably more important) is more informative to the reader (this is the language used in the paper, otherwise it is a bit confusing what doesoutputrefer to -- is it the output of the model? of mlp hidden layers?embedding_dimis more informative)