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.

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

Add recommend() function for the base class of Recommender #538

Merged
merged 68 commits into from
Oct 30, 2023

Conversation

tqtg
Copy link
Member

@tqtg tqtg commented Oct 25, 2023

Description

Adding recommend() to generate top-K item recommendations for a given user. Key difference between this function and rank() function is that rank() works with mapped user/item index while this function works with original user/item ID. This helps hide the abstraction of ID-index mapping, and make model usage and deployment cleaner.

In addition, we update the uid_map and iid_map in Dataset object to point to the global ones. It requires some changes for previous usages in some models. Now, train_set/val_set/test_set all hold references to the global mapping.

BREAKING CHANGE: any model should be self-contained and be able to make prediction without train_set dependency. This will simplify model saving and deployment process. In order to make this possible, we update the models to hold certain attributes which were previously kept in the train_set (e.g., num_users, num_items, uid_map, iid_map). Also, models should not hold reference to train_set/val_set but only use them during training.

After making the changes, I have tried to test all models to make sure they're runnable using the examples. However, I might miss certain details and would appreciate help from everyone to cross check, especially for the models that you've developed or you're familiar with.

Related Issues

Checklist:

  • I have added tests.
  • I have updated the documentation accordingly.
  • I have updated README.md (if you are adding a new model).
  • I have updated examples/README.md (if you are adding a new example).
  • I have updated datasets/README.md (if you are adding a new dataset).

@darrylong
Copy link
Member

is this a breaking change? :)

@tqtg
Copy link
Member Author

tqtg commented Oct 25, 2023

It shouldn't initially, but now it seems to be :)

Previously, our train_set didn't hold the reference to global mapping of user ID and item ID, but only the evaluation method. It works just fine if everything is under an Experiment, though I think we need to make this change in order to support future serving/model deployment.

Copy link
Member

@hieuddo hieuddo left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@hieuddo hieuddo self-requested a review October 26, 2023 06:45
@tqtg
Copy link
Member Author

tqtg commented Oct 26, 2023

Please feel free to clone this branch and push changes. Sorry for many updates in this single PR, though if we break it down the models will not be working as expected.

@hieuddo hieuddo requested a review from lthoang October 30, 2023 07:53
@tqtg tqtg merged commit edc83aa into PreferredAI:master Oct 30, 2023
12 checks passed
@tqtg tqtg deleted the recommend-API branch October 30, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants