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

Remove InferenceNode after migrating its functionality into the Node class from Merlin Core #379

Merged
merged 4 commits into from
Jul 12, 2023

Conversation

jperez999
Copy link
Collaborator

This PR removes the inference node and operator and moves the model registry apparatus to core. Now all operators are either base operator or stat operator. This is the first step in making all operators work in workflows and ensembles alike.

This PR requires the following core PR: NVIDIA-Merlin/core#357

@jperez999 jperez999 added the enhancement New feature or request label Jul 6, 2023
@jperez999 jperez999 added this to the Merlin 23.07 milestone Jul 6, 2023
@jperez999 jperez999 requested a review from karlhigley July 6, 2023 01:20
@jperez999 jperez999 self-assigned this Jul 6, 2023
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Documentation preview

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

# Make a new subclass of InferenceOperator so the mocks don't interfere with other tests.
class InferenceOperatorWithModelPath(InferenceOperator):
pass
# Make a new subclass of BaseOperator so the mocks don't interfere with other tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make the test pass, but now this functionality only exists in the tests. Unless we can figure out a way to put it the non-test code, we might just want to drop it for now. Writing out the actual code in the method every time you want to do this isn't great, but it's not that much and we can come back to this to make it more convenient once we have a better handle on how model registries should integrate with the rest of Merlin.

@karlhigley karlhigley changed the title Rm inference node Remove InferenceNode after migrating its functionality into the Node class from Merlin Core Jul 6, 2023
@jperez999 jperez999 merged commit 00a606f into NVIDIA-Merlin:main Jul 12, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants