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

[Serving] Add flow topology support #621

Merged
merged 13 commits into from Dec 29, 2020
Merged

Conversation

yaronha
Copy link
Collaborator

@yaronha yaronha commented Dec 28, 2020

  • add support for flows/DAG in serving runtime
  • enhance the graph, support streams, multi-function, ..
  • work with Storey as async runtime engine

Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

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

Looks good
The main thing I have to say is that without very high test coverage no one but you will be able to touch that code.
This PR does add very nice coverage, It was hard for me to follow and verify exactly what covered and what's not, if there is something you know that is not, please add
Also I highly suggest system tests to the whole serving thing (doesn't have to be part of this PR, but we must add it at some point.

mlrun/runtimes/serving.py Outdated Show resolved Hide resolved
mlrun/runtimes/serving.py Outdated Show resolved Hide resolved
mlrun/runtimes/serving.py Outdated Show resolved Hide resolved
mlrun/runtimes/serving.py Outdated Show resolved Hide resolved
elif topology == StateKinds.flow:
self.spec.graph = RootFlowState(engine=engine)
else:
raise ValueError(f"unsupported topology {topology}, use 'router' or 'flow'")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise ValueError(f"unsupported topology {topology}, use 'router' or 'flow'")
raise mlrun.errors.MLRunInvalidArgumentError(f"unsupported topology {topology}, use 'router' or 'flow'")

This is repeating, please replace in other places as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

tests/serving/test_async_flow.py Outdated Show resolved Hide resolved
tests/serving/test_async_flow.py Outdated Show resolved Hide resolved
tests/serving/test_flow.py Outdated Show resolved Hide resolved
tests/serving/test_serving.py Outdated Show resolved Hide resolved
tests/serving/test_serving.py Outdated Show resolved Hide resolved
@Hedingber Hedingber changed the title serving-flow [Serving] Add flow topology support Dec 29, 2020
Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

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

  • Change graphviz requirement to ~=0.16.0
  • you changed ValueError only in 2 places, but you have many other places in the code you're adding it, replace to mlrun.errors...
  • you wanted to rename self._error_stream no ?
  • you change g to graph in _add_graphviz_flow but not in _add_graphviz_router
  • I think that just like you changed > to $queue you should change * to something more meaningful
  • If you've left the flow.plot everywhere, get it out of comment, change the path to be under from tests.conftest import results and add a comment above it "plotting the graph to help debugging"
  • test_on_error seems duplicated, can be parameterized as well, also I would move the tests that are parameterized to a different file

@yaronha
Copy link
Collaborator Author

yaronha commented Dec 29, 2020

@Hedingber

  • i can lock graphviz but u can see they have a ver every month
  • re the * for router its different, its a prefix (e.g. *Ensemble) so cant change it to a name, people can always place the class as first arg instead

@Hedingber Hedingber merged commit de5ba23 into mlrun:development Dec 29, 2020
@yaronha yaronha deleted the serving-flow branch September 7, 2021 19:05
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.

None yet

2 participants