Review getting started examples

[radekosmulski]

No description provided.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[github-actions]

Documentation preview

https://nvidia-merlin.github.io/Merlin/review/pr-779

[radekosmulski]

rerun tests

[bschifferer]

We need to move them there for inference. Can we add the reason why we move them to it?

Reply via ReviewNB

[radekosmulski]

rephrased this explaining the situation to the reader

[bschifferer]

I think the paragraph doesn't make much sense

Applying deep learning models to recommendation systems faces unique challenges in comparison to other domains, such as computer vision and natural language processing. The datasets and common model architectures have unique characteristics, which require custom solutions. Recommendation system datasets have terabytes in size with billion examples but each example is represented by only a few bytes. For example, the Criteo CTR dataset, the largest publicly available dataset, is 1.3TB with 4 billion examples. The model architectures have normally large embedding tables for the users and items, which do not fit on a single GPU.

We do not address large embeddings in the example - I dont know why we mention it.

We do not use Criteo dataset, not sure why we explain something related to it?

CAn we rename NVTabular dataloader to Merlin dataloader?

This notebook explains, how to use the NVTabular dataloader to accelerate TensorFlow training.

Reply via ReviewNB

[radekosmulski]

completely changed this paragraph now

[bschifferer]

Are we using all imports? E.g. I dont think we will use workflow_fit_transform in the notebook?

Reply via ReviewNB

[radekosmulski]

removed unused imports

[bschifferer]

Can we add the tags to the previous notebook ( 02 NVTabular)?

I see that we add another NVTabular workflow in this notebook - which just adds tags to the workflow. I think it will be cleaner, if we add the tags in the previous notebook and do not require two NVTAbular workflows.

We can remove the gerne columns from the schema file to not load/create an archtecture with the feature

Reply via ReviewNB

[radekosmulski]

done

[bschifferer]

We can move the export code to the 4th notebook?

We can save the TensorFlow model.

In the 4th notebook, we can load the NVTabular workflow and TensorFlow model to create the graph with Merlin Systems

Reply via ReviewNB

[bschifferer]

We walked through the documentation, but it is useful to understand the API.

This sentence is not correct/uptodate as we do not walk through the documentation anymore. We can just delete it and write something like.

We define our model in a model.py file (+add reason why we do it in a model.py file)

Reply via ReviewNB

[radekosmulski]

done

[bschifferer]

@radekosmulski is it ready for a 2nd review? I am not sure, if you added the changes to the TF notebook

[bschifferer]

We need to move the tests to Merlin/tests/unit/examples/

[rnyak]

Line #2.        "INPUT_DATA_DIR", os.path.expanduser("~/nvt-examples/movielens/data/")

Is it possible to not have this path as root? is there a specific reason we set this as root? I know this is from existing examples, but I really dont like that this is in root :) can we set it as /workspace/nvt-examples/... if it wont create too much work? what do you think?

Reply via ReviewNB

[radekosmulski]

changed the path as advised

[rnyak]

@radekosmulski was changing path created any issue with the unit tests? let's be sure it wont :)

[rnyak]

Line #8.        write_hugectr_keyset=True  # only needed if using this ETL Notebook for training with HugeCTR

if not using HugeCTR, can we still keep this line or it should be removed? if it should be removed we can add a sentence to tell so.

Reply via ReviewNB

[radekosmulski]

added explanation

[rnyak]

10x speed up in training? with what dataset and model?DLRM? do you mind to give just one line more explanation or link to prove that statement? thanks.

Reply via ReviewNB

[radekosmulski]

This information was there in the notebook, someone else is stating that they saw this improvement, I have not come across any supporting evidence.

Should we keep it as is? otherwise can remove the 10x? or the whole passage altogether?

[rnyak]

@bschifferer do you know any supporting evidence/link of 10X speed up? thanks.

[rnyak]

Line #1.    %%writefile train_hugeCTR.py

Looks like the file name is not model.py here. it is train_HugeCTR.py so better to modify the file name in the sentence above: We will write the model to ./model.py and execute it afterwards

Reply via ReviewNB

[radekosmulski]

corrected

[rnyak]

I am not sure this The second tensor is a supporting Tensor, describing how many genres are associated with each datapoint in the batch. sentence is really accurate. why because according to my understanding from this sentence, I see that second tensor has 88846 value and but a batch in this example cannot have a single tensor of that amount of data points.

I'd say the second tensor is representing the starting point of each tensor of genres, and the difference between two successive number shows the number of datapoints in each tensor.

basically, can we clarify it better? what do you think?

Reply via ReviewNB

[radekosmulski]

Absolutely! Made the change as requested

[rnyak]

is it really the length or more like starting point?

Reply via ReviewNB

[radekosmulski]

made the change

[rnyak]

Line #6.        train_loss, y_pred, y = process_epoch(train_loader,

I recommend you set the loss as BCE because the process_epoch's default loss func is MSE loss which does not make sense for a binary classification problem: https://github.com/NVIDIA-Merlin/NVTabular/blob/main/nvtabular/framework_utils/torch/utils.py#L64

so training a model with wrong loss wont be accurate. what do you think?

Reply via ReviewNB

[radekosmulski]

that's a good point, change made

[rnyak]

it should be merlin-tensorflow container.

Reply via ReviewNB

[radekosmulski]

corrected the text and the url

[rnyak]

Line #6. configure_tensorflow()

are you sure you want to use configure_tensorflow() it automatically takes 50% of the GPU memory.. instead you could use os.environ["TF_GPU_ALLOCATOR"]="cuda_malloc_async".

Reply via ReviewNB

[radekosmulski]

replaced as suggested

[rnyak]

May be explain what's DCN briefly and add the image here? you can steal information from this notebook :) https://github.com/NVIDIA-Merlin/models/blob/main/examples/03-Exploring-different-models.ipynb

Reply via ReviewNB

[radekosmulski]

explanation + image added! 🙂

[rnyak]

Line #5. prediction_tasks=mm.BinaryClassificationTask(target_column),

we no longer use mm.BinaryClassificationTask instead we use prediction_tasks=mm.BinaryOutput(target_column) .

Reply via ReviewNB

[radekosmulski]

change made

[radekosmulski]

unfortunately, the serving operator doesn't work if I use mm.BinaryOutput

 ---------------------------------------------------------------------------
InferenceServerException                  Traceback (most recent call last)
Cell In [11], line 9
      3 outputs = [
      4     grpcclient.InferRequestedOutput(col)
      5     for col in ["rating/binary_classification_task"]
      6 ]
      8 with grpcclient.InferenceServerClient("localhost:8001") as client:
----> 9     response = client.infer("predicttensorflow", inputs, request_id="1", outputs=outputs)
File /usr/local/lib/python3.8/dist-packages/tritonclient/grpc/init.py:1431, in InferenceServerClient.infer(self, model_name, inputs, model_version, outputs, request_id, sequence_id, sequence_start, sequence_end, priority, timeout, client_timeout, headers, compression_algorithm)

1429     return result

1430 except grpc.RpcError as rpc_error:

-> 1431     raise_error_grpc(rpc_error)
File /usr/local/lib/python3.8/dist-packages/tritonclient/grpc/init.py:62, in raise_error_grpc(rpc_error)

61 def raise_error_grpc(rpc_error):

---> 62     raise get_error_grpc(rpc_error) from None
InferenceServerException: [StatusCode.INVALID_ARGUMENT] unexpected inference output 'rating/binary_classification_task' for model 'predicttensorflow'

[rnyak]

@radekosmulski if we can figure out what's the correct output name in your config file (please check it out) you can replace it in the for col in ["rating/binary_classification_task"] line here. if config files do not show the output name, we can checkout from model.

[rnyak]

Actually it is more than that. triton_op.export is exporting config files as well right?

If yes, I'd say better to write down below what happens after export. We generate model config files so that we can load our models on TIS and send requests successfully. t

Reply via ReviewNB

[radekosmulski]

clarification added

[rnyak]

Please remove this code docker run -it --gpus device=0 .. line . this is no longer the case. we are in the merlin-tensorflow container which does serving as well. so no need to launch a new container. Please remove the sentence Before we get started, you should start the container for Triton Inference Server with the following command. This command includes the -v argument that mounts your local model-repository folder with your saved models from the previous notebook (03-Training-with-TF.ipynb). 

Reply via ReviewNB

[radekosmulski]

done

[rnyak]

you can write down as We need to start the Triton Inference Server first. We can do that with the following command. You need to provide correct path for the models directory. Note that since we add --model-control-mode=explicit flag the models wont be loaded yet, we will load our model below. 

Reply via ReviewNB

[radekosmulski]

change made

[rnyak]

is this transformed dataset? if you are only serving TF model not NVT + TF then we should send the transformed dataset as a request, not the raw. Can you add clarification please saying that we send transformed dataset (if this is the case) due to such such reason? if this is not transformed dataset, then this is not accurate, if you are only serving the model.

Reply via ReviewNB

[radekosmulski]

yes, this is the transformed dataset, the one our model was trained on, will add clarification

[rnyak]

remove this. we dont need to launch the container again. we are already in the container.

Reply via ReviewNB

[radekosmulski]

done

[rnyak]

remove this line.

Reply via ReviewNB

[radekosmulski]

done

[rnyak]

you can remove After you started the container .Just say We can launch the Triton Inference Server with the following command:

Reply via ReviewNB

[radekosmulski]

done

[rnyak]

you can say since we add --model-control-mode=explicit flag, the model wont be loaded at this step, we will load the model below.

Reply via ReviewNB

[radekosmulski]

done

[radekosmulski]

rerun tests

[rnyak]

there is a typo here in the give 

Reply via ReviewNB

[radekosmulski]

thank you, corrected!

[rnyak]

wont --> will not

Reply via ReviewNB

[radekosmulski]

fixed!

[rnyak]

Serve --> Server

Reply via ReviewNB

[radekosmulski]

fixed

[radekosmulski]

rerun tests