Remove the legacy Getting Started With MovieLens example notebooks #859
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Documentation preview |
|
@karlhigley thanks for the PR. I am fine to remove the legacy example nbs. What are the updates required in |
|
@rnyak Good catch! I didn't even notice that was in the changed files—I didn't do anything to it intentionally, so I suspect those are formatting changes performed by the pre-commit hooks. We might want to run the pre-commit hooks on all the files in the repo, but that doesn't need to be in this PR. I'll remove it. |
| @@ -85,75 +85,6 @@ def run_triton_server(modelpath): | |||
|
|
|||
| # pylint: disable=unused-import,broad-except | |||
There was a problem hiding this comment.
we keep the test_z_legacy_notebooks for a specific purpose? do we still need that unit test? I am asking not that I know the answer :)
There was a problem hiding this comment.
This file came from NVT and had a bunch of example notebook tests in it, but I'm not sure if we still need it either. I removed the tests that looked directly related to the MovieLens example, but if we remove more of the example notebooks maybe we can remove the rest of this too?
These notebooks have already been reworked and updated to use more recent versions of Merlin, and the tests for these notebooks are starting to cause maintenance headaches. Seems safe to get rid of them at this point to save ourselves the hassle of continuing to keep them running.
karlhigley
force-pushed
the
cleanup/movielens-example
branch
from
31a35db
to
f1b753c
Compare
These notebooks have already been reworked and updated to use more recent versions of Merlin, and the tests for these notebooks are starting to cause maintenance headaches. Seems safe to get rid of them at this point to save ourselves the hassle of continuing to keep them running.
(The failing tests related to this legacy example are blocking the CI container build, which is partially blocking the Models PR to update the input format, which is itself blocking downstream work on session-based serving, so... 🤪 )