Draft: Add skip decorator; A few clean ups #306
Conversation
cloudw
changed the title
| def __init__(self, check="", next=""): | ||
| super().__init__() | ||
| self.check = check | ||
| self.next = next |
There was a problem hiding this comment.
The feature ask is to skip a step.
Introducing conditional branching (a conditional next step -> conditional branching) is out of scope.
There are valid deep reasons to not allow conditional branching; the mainly that upsteam Metaflow OSS has not implemented this feature because conditional branching introduces for complex DAG reasoning and branching that is difficult to test and reason over for the applied scientist. For example, this next conditional could goto any step.
There was a problem hiding this comment.
This is the version that RMX is using right now: https://gitlab.zgtools.net/analytics/artificial-intelligence/rmx/workflows/rmx-delg-eventing/-/merge_requests/43/diffs
It's also the same version provided by Ville in https://zillowgroup.slack.com/archives/C02116BBNTU/p1660145168960519?thread_ts=1660134373.601939&cid=C02116BBNTU
There was a problem hiding this comment.
Thank you for doing this. I think it's worth investigating whether this should be a true step decorator, or a normal pythonic decorator. I highly recommend keeping this simple by including the example @skip python function decorator and not a Metaflow decorator as exemplified here. That would limit the scope of this work item.
Introducing DAG conditionals is out of scope due to the complexities it introduces (and reduces portability of Metaflow) while diverges this branch (WFSDK) from Metaflow OSS.
| self.check = check | ||
| self.next = next | ||
|
|
||
| def __call__(self, f): |
There was a problem hiding this comment.
How and when is __call__ invoke?
I believe that this conflating this Metaflow compiler step decorator with a python function decorator.
It does feel like the StepDecorator is complicating the implementation a bit, that's why I'm marking this PR as draft right now. I think the done criteria is not too clear either. I'll keep this PR as draft for longer without working on it, until this becomes a priority. |
Changes:
Fix AIP-8494