-
Notifications
You must be signed in to change notification settings - Fork 24
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
Rewrite Choice step to make it usable from mlrun #537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not the person who should review it, but if I may, it looks pretty much straight forward. I had a comment and few suggestions but besides these, A+ :)
self.to(default) | ||
self._default = default | ||
def _init(self, **kwargs): | ||
super()._init() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure it is not an error: before the change the kwargs
were sent at the Flow
init so I would expect to see super().__init__(**kwargs)
unless it is redundant. I think the outlets
param should be passed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the **kwargs
need to be removed. There's no kwargs in _init()
. Note that _init
is unrelated to __init__
.
storey/flow.py
Outdated
# TODO: hacky way of supporting mlrun preview, which replaces targets with a DFTarget | ||
self._passthrough_for_preview = list(self._name_to_outlet) == ["dataframe"] | ||
|
||
def select_outlets(self, event): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a type hint and also in the doc string mention that the selected outlets should be returned in a list of strings - the outlet names.
def select_outlets(self, event): | |
def select_outlets(self, event) -> List[str]: |
if self._passthrough_for_preview: | ||
outlet = self._name_to_outlet["dataframe"] | ||
outlets.append(outlet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a TODO here as well to remember to delete this "hack"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really want to duplicate the TODO that's already on the attribute definition.
ML-7818