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

Move Next-Item-Prediction-with-Transformers examples from models #1028

Merged
merged 6 commits into from
Jul 5, 2023

Conversation

radekosmulski
Copy link
Contributor

This is a part of larger ongoing work that we are tracking under #1027.

Here I am only moving the examples from models to Merlin and making sure the tests run. Once this is in place, I'll work on merging a PR in models to remove the examples and clean up tests, etc.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@radekosmulski radekosmulski added the examples Adding new examples label Jun 27, 2023
@github-actions
Copy link

Documentation preview

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

timeout=720,
execute=False,
)
@pytest.mark.notebook
Copy link
Contributor

Choose a reason for hiding this comment

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

This throws a warning (not an error) in the logs:

tests/unit/test_transformers_next_item_prediction_with_pretrained_embeddings.py:21
  /home/gha-user/gha1/Merlin/Merlin/tests/unit/test_transformers_next_item_prediction_with_pretrained_embeddings.py:21: PytestUnknownMarkWarning: Unknown pytest.mark.notebook - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
    @pytest.mark.notebook

You can add a notebook marker here: https://github.com/NVIDIA-Merlin/Merlin/blob/main/pytest.ini to stop the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you ver much for looking at this @nv-alaiacano! 🙏 will make the changes today

The GPU CI pipeline that was failing for me the whole day (which I posted about in slack) passed on the last run! (I left it running as I shut down for the day but had no hope it would pass as it failed with the same error for me 5+ times yesterday, but turns out it now passes!)

Thank you very much for looking into this anyhow and for spotting this issue and pointing me how to get rid of the warnings, will do so today 🙂

@radekosmulski radekosmulski force-pushed the move_examples_from_merlin_models branch from cfb9764 to 2b21651 Compare June 27, 2023 22:43
@bschifferer
Copy link
Contributor

What are the changes in [01-Building-Recommender-Systems-with-Merlin.ipynb](https://github.com/NVIDIA-Merlin/Merlin/pull/1028/files#diff-ae3151beaa9bd704ff7c957b3a35394e15f72122a4746c79ea132feb02e645eb) - I checked the diff with ReviewNB but it seems there are no changes (except of output prints). If that is the case, should we remove it from the PR?

@radekosmulski radekosmulski force-pushed the move_examples_from_merlin_models branch from 151fea3 to 67b681a Compare June 29, 2023 04:28
@radekosmulski
Copy link
Contributor Author

Thank you very much for catching this, Benedikt! My bad. I have now removed the file.

* Move over the examples
* Move over tests (along with the asvdb benchmark tracking test)
* Add `notebook` marker to pytest.ini
* Update tracking logo on the example with pretrained embeddings
@radekosmulski radekosmulski force-pushed the move_examples_from_merlin_models branch from 67b681a to b85747b Compare June 29, 2023 04:29
@bschifferer bschifferer merged commit ad0c395 into main Jul 5, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Adding new examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants