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

Yakov/mlpiper restful refactoring #1097

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yakov-g
Copy link
Collaborator

@yakov-g yakov-g commented Aug 1, 2024

This repository is public. Do not put here any private DataRobot or customer's data: code, datasets, model artifacts, .etc.

Summary

@klichukb @eric-s-s @yura-ant
Here is the move of the component from mlpiper to drum.
I had to move a bit more extra files than I expected.
Most of these dependencies are there to make this things more generic and likely can be very simply removed.

This component is covered by functional tests, so this move works fine.

Would like to here your suggestions about this move? Do we want to merge it?

Rationale

@yakov-g yakov-g force-pushed the yakov/mlpiper-restrul-refactoring branch from 9ef833d to c1fb792 Compare August 1, 2024 23:36
@yakov-g yakov-g changed the title Yakov/mlpiper restrul refactoring Yakov/mlpiper restful refactoring Aug 1, 2024
@yakov-g yakov-g requested review from klichukb and removed request for klichukb August 2, 2024 04:02
@yura-ant
Copy link
Contributor

yura-ant commented Aug 2, 2024

That PR has just one unit test which I've added when implement ability to configure threads option in mlpiper. This is definitely not enough for such big changes

As reviewer I should be able to read unit tests to understood how proposed new functionality works and just after that review how it was implemented.

Some existing functional tests (which exactly are not clear from PR) in DRUM cover how mlpiper was integrated before and they not test all those classes which you plan to add. Those test are good as regression test to prove that expected integration with mlpiper continue works as before.

Basic rule is when you add a new function you should add unit test.
Once we follow it we have a lot benefits

Also this PR introduce a lot of duplicated classes one from mlpiper another one form drum which could lead to issues if user chose wrong import. Ideally if we remove completely dependency to mlpiper or remove those classes from mlpiper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants