Skip to content

Conversation

@karlhigley
Copy link
Contributor

@karlhigley karlhigley commented Dec 29, 2022

This code was copied from NVTabular with the intention of using it as a starting point for refactoring in the direction of an operator-based serving API (which now mostly exists), but the remaining unused code was never removed. This PR removes it an effort to avoid confusing users with multiple versions of the legacy serving code they could (attempt to) use.

See also:

This code was copied from NVTabular with the intention of using it as a starting point for refactoring in the direction of an operator-based serving API (which now mostly exists), but the remaining unused code was never removed. This PR removes it an effort to avoid confusing users with multiple versions of the legacy serving code they could (attempt to) use.
@github-actions
Copy link

Documentation preview

https://nvidia-merlin.github.io/systems/review/pr-265

@nv-alaiacano nv-alaiacano added the breaking Breaking change label Dec 29, 2022
@karlhigley
Copy link
Contributor Author

karlhigley commented Dec 30, 2022

This shouldn't cause downstream issues, because this library doesn't seem to have users yet, because these methods have been omitted from our documentation, because we haven't (AFAIK) been using them in examples or otherwise doing anything to indicate that they exist or should be used.

Whether this constitutes a breaking change or not is a semantic issue I don't care to debate at any length, but please understand that avoiding changes to the legacy serving code that would impact our users is why this code existed here (in addition to in NVT) in the first place. We've been so cautious with avoiding breaking changes that we've caused a new issue, which is the proliferation of similar but slightly different versions of the same code in multiple places, which is confusing enough that it's even tripping up our own team. It's time that we decide on one place for the legacy serving code to live, and then bite the bullet on whatever changes we need to make to get there (breaking or otherwise.)

My suggestion would be to move it to a serving repo that contains only model serving code, in order to clearly separate that from more experimental system-level functionality that lives in this repo.

from shutil import copyfile

# this needs to be before any modules that import protobuf
os.environ["PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION"] = "python"
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can remove this and then we can remove all the # noqa comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking change chore Maintenance for the repository clean up

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants