Small fixes in getting-started ETL and training notebooks and fix tuple error in serving notebook#586
Small fixes in getting-started ETL and training notebooks and fix tuple error in serving notebook#586karlhigley merged 8 commits intomainfrom
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Documentation previewhttps://nvidia-merlin.github.io/Transformers4Rec/review/pr-586 |
sararb
left a comment
There was a problem hiding this comment.
Thank you for the PR! The updates look good to me. I just left one comment regarding the old notebooks' unit test.
| @pytest.mark.skipif(find_spec("cudf") is None, reason="needs cudf") | ||
| # @pytest.mark.skipif(find_spec("cudf") is None, reason="needs cudf") | ||
| @pytest.mark.skip(reason="there is a testbook version of this test") | ||
| def test_session(tmpdir): |
There was a problem hiding this comment.
To double-check, will this test always be skipped?
If yes, can we remove it as it is replaced by the testbook version?
There was a problem hiding this comment.
@jperez999 since I am replacing this test with the testbook version I added mark.skip into the existing test, but we can remove it as well. what do u think? thanks.
|
The PR looks good to me. Do we have no Jenkins GitHub Action for it? |
Thanks for reviewing. If I understood your question correctly : AFAIK, we do no longer use Jenkins for PR testing |
|
it ran the new test and skipped the old one https://github.com/NVIDIA-Merlin/Transformers4Rec/actions/runs/3795493384/jobs/6454668622#step:3:326 |
This PR:
03notebook.01and02notebooks using testbook.Note: if we want a unit test for serving notebook, we would need systems here. currently we dont couple TF4Rec to systems, that's why I removed that part in the unit test code. The decision is to do that in Merlin repo, instead of TF4Rec repo.