-
Notifications
You must be signed in to change notification settings - Fork 729
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
added local decorator #1824
base: master
Are you sure you want to change the base?
added local decorator #1824
Conversation
076aeb5
to
1cbf148
Compare
metaflow/decorators.py
Outdated
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
1cbf148
to
91d9080
Compare
@romain-intel @savingoyal I have updated the PR with the should attach class method approach. Please review once. |
@vardhaman-surana, thanks for the PR. I would like to understand the use case better. Currently, we have One possible workaround is to maybe have |
hi @savingoyal the use case as mentioned in the issue is A possible use-case for this would be to annotate a GPU-heavy step that you want to run locally to save on AWS costs but take advantage of fast S3 access for other steps (from the EC2 machines batch uses). i tried adding the local decorator based on this requirement mentioned in the issue. I am not sure if the env variable approach which you are suggesting will work for this case or not. |
for issue: #350
@local
--with batch
and--with kubernetes
, by preventing attachment of batch and kubernetes decorators to the steps which have@local
attached to them already.to achieve this: