Skip to content

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.

Sign up for GitHub

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

Merged
claudiazi merged 5 commits into from
Jun 24, 2025
Merged

Feature/data 2275 adjsut soda to deferrable #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd80d5e
feat: convert soda to deferrable
Jun 18, 2025
289af26
Merge remote-tracking branch 'origin/master' into feature/DATA-2275-a…
Jun 18, 2025
f961d15
feat: convert soda to deferrable
Jun 19, 2025
1649376
feat: convert soda to deferrable
Jun 19, 2025
fca31a6
fix: add extra package
Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dagger/dag_creator/airflow/operator_creators/soda_creator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def _create_operator(self, **kwargs):
job_queue=self._task.job_queue,
container_overrides=overrides,
awslogs_enabled=True,
deferrable=True,
Copy link
Member

@siklosid siklosid Jun 19, 2025

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?

Copy link
Author

@claudiazi claudiazi Jun 19, 2025

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?

Copy link
Member

@kiranvasudev kiranvasudev Jun 19, 2025

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?

Copy link
Author

@claudiazi claudiazi Jun 19, 2025

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 🙈.

Copy link

@raimundovidaljunior raimundovidaljunior Jun 19, 2025 •
edited
Loading

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

Copy link
Member

@kiranvasudev kiranvasudev Jun 19, 2025

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

Copy link
Author

@claudiazi claudiazi Jun 19, 2025

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

Copy link
Author

@claudiazi claudiazi Jun 19, 2025

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. 🙈

Copy link

@antruo antruo Jun 19, 2025

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: 🙈

**kwargs,
)
return batch_op
1 change: 1 addition & 0 deletions reqs/base.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
aiobotocore>=2.5.0
click==8.1.3
croniter==2.0.2
envyaml==1.10.211231
Expand Down