added local decorator #1824
added local decorator #1824
Conversation
vardhaman-surana
force-pushed
the
local-decorator
branch
from
076aeb5
to
1cbf148
Compare
| @@ -481,6 +483,12 @@ def _attach_decorators_to_step(step, decospecs): | |||
| deconame = splits[0] | |||
| if deconame not in decos: | |||
| raise UnknownStepDecoratorException(deconame) | |||
|
|
|||
There was a problem hiding this comment.
I have a few issues with this and I don't, unfortunately, have an ideal solution for it yet. I don't want to put specific decorators here (in core). There are other decorators that local could impact (for example, at Netflix, we have a @titus decorator). Ideally we would have a better way for decorators to communicate between each other (or something).
One thing that I think may be better (but @savingoyal could also comment) is actually putting stuff in the batch and kubernetes decorator and have them "disable" themselves if they see that local is present. I think that type of change may be more palatable and puts less stuff in the core.
There was a problem hiding this comment.
I can work on implementing a solution to have batch and Kubernetes operators disable themselves when local decorator is present. Should I proceed with this approach?
There was a problem hiding this comment.
if i don't put condition directly in the attach_decorators function and add a method (let's say should_attach(step)) in base StepDecorator which always return true.
and override that in BatchDecorator and KubernetesDecorator class to check for the local decorator presence...
would that be acceptable ?
and that method can be evaluated in attach_decorators before attaching any decorator.
Again this will be a change in the core but i guess it will make the chances of breaking other things less.
There was a problem hiding this comment.
I'll let @savingoyal chime in. I think the least intrusive one is just:
- add a local decorator
- have the batch/kube/etc decorators check for the presence of the local decorator and disable themselves if present
This doesn't add much to core (apart from the local decorator) and we can then properly evolve it. We are looking at redesigning the decorator interfaces as well so this can probably be a part of that. In the meantime, this seems like a small enough solution. We could even get a tad fancier and check if the decorators are static or not (added with --with batch) and error if there is local and batch statically there.
There was a problem hiding this comment.
@savingoyal should i proceed with adding should_attach function in stepDecorator ?
@romain-intel i was trying to disable batch and kubernetes decorators from within themselves but it seems that approach will require changes in each function of those decorators which are called as per the workflow of decorators.
for issue: #350
@local--with batchand--with kubernetes, by preventing attachment of batch and kubernetes decorators to the steps which have@localattached to them already.