-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/data 2275 adjsut soda to deferrable #56
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
Feature/data 2275 adjsut soda to deferrable #56
Conversation
| job_queue=self._task.job_queue, | ||
| container_overrides=overrides, | ||
| awslogs_enabled=True, | ||
| deferrable=True, |
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.
Why do we need to change dagger code? Woudln't it be enough to add deferrable=True to the task parameters in the yaml config of the Soda tasks?
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.
But if we want to make all the soda tasks to deferrable. then the fastest way to do it would be on dagger code?
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.
agree with David. if we already can do this on the task config template, why not set it there as a default?
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.
but if we want to set all soda tasks as deferrable i dont see why its better to set it on the template instead of the code level 🙈.
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.
but if we want to set all soda tasks as deferrable i dont see why its better to set it on the template instead of the code level 🙈.
I agree, if the default behaviour we want is to be deferrable I wouldn't see why not to put here, why would it be better to put it in all yamls @kiranvasudev @siklosid instead of here ? maybe I am not understanding something
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.
some reasons i can think of why its better on template than code:
- Keeps dagger as a library generic instead of specific.
- I guess it also aligns with Dagger’s design where templates pass parameters down to tasks. Since we want “deferrable by default”, we can still do it via a shared base template, which is easier than rolling back code changes if something breaks
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.
but setting deferrable by default for the soda task is still generic?
just like we set all the sensors as defaultly deferrable
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 didnt test it with just adding the task_parameter deferrable: True on the template 🙈
But anyways we have to add the package aiobotocore in the requirements of dagger to make this parameter work. 🙈
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.
@claudiazi most used emoji ever: 🙈
Convert soda task to deferrable task so that it wont take the spot of the active parallel tasks.
aiobotocoreis needed for aws async