may be we can add some text right above this cell to explain what we are doing here and what we are registering to feast repo. I can do that.

Reply via ReviewNB

we need to explain this cell what's going on here, since cat_wkflow = nvt_wkflow.get_subworkflow("items_cat") is a new syntax..

Reply via ReviewNB

Line #5.        view="user_features",

guess view should be user_attributes ?

Reply via ReviewNB

@jperez999 and @karlhigley changing view to user_attributes give error since it was saved to feature store as user_features. so we should leave it as it is.

yeah you cant change the name of the view, that comes from the feast configs at the bottom of notebook 1.

You can change it if it’s changed in both places though, right?

Line #3.        view="item_features",

similarly, here view should be item_attributes? but if we use "item_attributes" it gives error since we registered it at item_features in the feature store.

Reply via ReviewNB

Same as above for this change. You cannot make this change unless we change the configs.

I think we probably should change both for the sake of clarity

Line #14.    os.environ["TF_GPU_ALLOCATOR"] = "cuda_malloc_async"

guess we need to move this line right after import os to make it effective.

Reply via ReviewNB

As long as import os is done before it does not have to be right before. If you look further up in that block os is the first import.

@jperez999 I wanted to say if os.environ["TF_GPU_ALLOCATOR"] = "cuda_malloc_async" comes after import tensorflow it might NOT work.

I think it has to happen in with the os call first and import tensorflow second so that TF picks up the config from the environment when it loads

I say let's move os.environ["TF_GPU_ALLOCATOR"] = "cuda_malloc_async" right after import os based on my experience and before import tensorflow :)

Line #5.    item_embs_df["item_id"] = item_features["item_id"]

I am skeptical that this step is correct. This is not doing correct mapping. if we are trying to replace encoded items with their original raw item_ids, I think, we should use unique.item_id.parquet. so we can do something like

raw_item_ids = pd.read_parquet('./categories/unique.item_id.parquet')
item_embs_df["item_id"] = raw_item_ids["item_id"].values

Reply via ReviewNB

In addition, are we sure we want to save item_embeddings for raw item_ids? Arent we supposed to do KNN search after retrieval stage based on the encoded items? If yes, we should then get rid of the mapping scripts above.

You need to save the embeddings with raw_item_ids because those embeddings should correspond to raw data. This way when you do the lookup in the second notebook you are getting the actual items. Those items are then processed through the item subworkflow to get the features for the model to rank the items. Suppose you wanted to use that similarity search with a different feature engineering workflow, if you had placed the data with the item ids already categorified that would not work. For the item_id to raw_item_ids you are correct that it is not correctly merged. However, I pose the question why are we sorting IDs by default? I see that we use it in the code for categorify to handle making na/nulls the first items in the dataframe... but as a user I think I expect to see the categories given in a first come first serve order. Unless I elect to use freq_threshold or kwarg that changes the strategy. Anyway I will fix for now.

Line #1.    user_id_raw = ["user_id"] >> Rename(postfix='_raw') >> LambdaOp(lambda col: col.astype("int32")) >> TagAsUserFeatures()

I think we no longer need user_id_raw and item_id_raw since we do not register them in the feature store.

Reply via ReviewNB

That is correct but I wanted to leave it so people can see the incremental changes. I will remove it.

Line #2.        store=feature_store,

I am not sure how this step works without registering any encoded ids in the feature store. I assume retrieval["candidate_ids"]  returns encoded item ids? then, how feature store can map encoded ids to the raw ids if it does not have such mapping info stored?

Reply via ReviewNB

Those candidate IDs come from an embedding space, that embedding space is loaded into a vector database to be able to conduct similarity search. The IDs returned here are the raw item IDs (aka the actual names) from the previous notebook (note they are currently misrepresented, because categorify always sorts the values in a dataframe, I will fix it shortly). So you take the user features and pass them to the retrieval model to get the user embeddings. Those embeddings are overlayed in the item embedding space and then we run a similarity search against those to find the items that are closest (most similar) to the user embedding supplied. Once we have the real item_ids (should be item names/uuid) we can query for the features of those items in the feature store. The records we end up with are real item id/feature values that have not been processed yet. That is why the next step is to process them using the item subworkflow.

@jperez999 as long as retrieval["candidate_ids"] return raw item_ids then all good.

This would be a lot easier to sort through if the dataset generation returned string ids for categorical columns, because we’d then be able to visually tell whether we were looking at raw or encoded values 🤔

@karlhigley agreed :)

I took a stab at adding string id columns to the dataset generation this morning, and quickly realized there are some bigger issues to sort out re: synthetic data and how our libraries relate to it. @oliverholworthy is going to take a look and try to figure out where we should go with that, but for now I think we're going to have to continue with integer IDs. I wonder if we could make them more visually distinct from the Categorified IDs if we added a large offset to the raw IDs (e.g. 1_000_000)?

I took a stab at adding string id columns to the dataset generation this morning, and quickly realized there are some bigger issues to sort out re: synthetic data and how our libraries relate to it. @oliverholworthy is going to take a look and try to figure out where we should go with that, but for now I think we're going to have to continue with integer IDs. I wonder if we could make them more visually distinct from the Categorified IDs if we added a large offset to the raw IDs (e.g. 1_000_000)?

@karlhigley if this is a nice to have things we dont have to prioritize it now. your call, I am ok with moving with integer IDs :)

Line #6.    schema = train.schema.without(['user_id_raw', 'item_id_raw'])

if we remove user_id_raw and item_id_raw above from NVT pipeline, we can remove .without here as well.

Reply via ReviewNB

Let's rephrase this as

"We start with the offline candidate retrieval stage. We are going to train a Two-Tower model for item retrieval using only positive interactions, that's why we apply Filter operator below. To learn ..."

Reply via ReviewNB

we already have a section "Feature Engineering with NVTabular" above. So better to remove this title here.

Reply via ReviewNB

Let move this paragraph up right under Feature Engineering with NVTabular section above.

Reply via ReviewNB

Line #3.    )

so we can save this as user_attributes if you want like below:

user_features.to_parquet(

  os.path.join(feature_repo_path, "data", "user_attributes.parquet")

)

Reply via ReviewNB

so we can save this as item_attributes if you want like below:

item_features.to_parquet(

  os.path.join(feature_repo_path, "data", "item_attributes.parquet")

)

Reply via ReviewNB

is that sending a request of transformed dataset instead of raw user_id?

Line #4.    workflow =  nvt.Workflow(["item_id"] + (['item_id', 'item_brand', 'item_category', 'item_shop'] >> TransformWorkflow(nvt_wkflow.get_subworkflow("item")) >> PredictTensorflow(model_tt.first.item_block())))

This step is not really intuitive from user perspective. we should explain why we have two times item_id? why we need to do fit_transform not only transform? why are we fitting again?

Reply via ReviewNB

This line and comment have us way down a rabbit-hole fixing underlying issues that make this awkward 😅