You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "nrobinson-intelycare (via GitHub)" <gi...@apache.org> on 2025/01/15 00:26:13 UTC
[PR] Add support for timeout to BatchOperator [airflow]
nrobinson-intelycare opened a new pull request, #45660:
URL: https://github.com/apache/airflow/pull/45660
Redo of https://github.com/apache/airflow/pull/45619 to avoid merge commit
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "nrobinson-intelycare (via GitHub)" <gi...@apache.org>.
nrobinson-intelycare commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2607492449
@o-nikolas @vincbeck
Any idea when a new release of [apache-airflow-providers-amazon](https://pypi.org/project/apache-airflow-providers-amazon/) will be made? Looking forward to using this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #45660:
URL: https://github.com/apache/airflow/pull/45660#discussion_r1917211935
##########
providers/src/airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -184,6 +185,7 @@ def __init__(
poll_interval: int = 30,
awslogs_enabled: bool = False,
awslogs_fetch_interval: timedelta = timedelta(seconds=30),
+ boto3_timeout: dict | None = None,
Review Comment:
I don't love the name `boto3_timeout` it's misleading because it's not really anything to do with boto3 (not some kind of SDK API request timeout). I'd be fine with just `timeout` and then in the param description describe that it's the `submit_job` API timeout. Or I'd also like `submit_job_timeout`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "nrobinson-intelycare (via GitHub)" <gi...@apache.org>.
nrobinson-intelycare commented on code in PR #45660:
URL: https://github.com/apache/airflow/pull/45660#discussion_r1915858017
##########
providers/src/airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -184,6 +185,7 @@ def __init__(
poll_interval: int = 30,
awslogs_enabled: bool = False,
awslogs_fetch_interval: timedelta = timedelta(seconds=30),
+ boto3_timeout: dict | None = None,
Review Comment:
Having an `int` for timeout seconds in the BatchOperator and then passing a dict on behalf of the user is a great idea. Would `boto3_timeout` still be a good name for the parameter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "nrobinson-intelycare (via GitHub)" <gi...@apache.org>.
nrobinson-intelycare commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2622868611
@eladkal Any chance of a release this week? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "nrobinson-intelycare (via GitHub)" <gi...@apache.org>.
nrobinson-intelycare commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2593946624
@o-nikolas @vincbeck @eladkal
Anything I'm missing blocking this from being merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas merged PR #45660:
URL: https://github.com/apache/airflow/pull/45660
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2607647249
Will have to wait after Celery release.
I don't have time to parallel releases. Sorry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "nrobinson-intelycare (via GitHub)" <gi...@apache.org>.
nrobinson-intelycare commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2594006801
> > @o-nikolas @vincbeck @eladkal
> > Anything I'm missing blocking this from being merged?
>
> Just curious, have you tested it manually when `submit_job_timeout` is not provided? In that case `timeout` is passed to the the api `submit_job` with value `{"attemptDurationSeconds": None}`. I am wondering it Boto3 is okay with that
I've changed it to only add `timeout` to the `args` dict only if submit_job_timeout is not None:
https://github.com/apache/airflow/pull/45660/files#diff-730ff9a9f5255ee440584231fc8733730c53a6a1a3fa3c2edd993c9d5157f219R322
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2593981457
> @o-nikolas @vincbeck @eladkal
>
> Anything I'm missing blocking this from being merged?
Just curious, have you tested it manually when `submit_job_timeout` is not provided? In that case `timeout` is passed to the the api `submit_job` with value `{"attemptDurationSeconds": None}`. I am wondering it Boto3 is okay with that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "nrobinson-intelycare (via GitHub)" <gi...@apache.org>.
nrobinson-intelycare commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2607713670
> Will have to wait after Celery release. I don't have time to parallel releases. Sorry.
No worries, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #45660:
URL: https://github.com/apache/airflow/pull/45660#discussion_r1915822800
##########
providers/src/airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -184,6 +185,7 @@ def __init__(
poll_interval: int = 30,
awslogs_enabled: bool = False,
awslogs_fetch_interval: timedelta = timedelta(seconds=30),
+ boto3_timeout: dict | None = None,
Review Comment:
From the docs, the timeout dict contains just one parameter: the seconds for the timeout. The AWS APIs can be strange and complicated, I wonder if this is an opportunity to simplify the Operator interface in comparison and simply accept an `int` here then put that in a dict on behalf of the user in execute()?
If you don't like that approach, then I'd vote for creating a general `submit_job_kwargs` arg, that the user can use to pass through _any_ kwargs the SubmitJobs AWS API supports. We have followed this pattern before, you can see some examples below:
- https://github.com/aws-mwaa/upstream-to-airflow/blob/6d048c43f0753d96976f3c9e72262cfe3b27d052/providers/src/airflow/providers/amazon/aws/operators/rds.py#L555
- https://github.com/aws-mwaa/upstream-to-airflow/blob/6d048c43f0753d96976f3c9e72262cfe3b27d052/providers/src/airflow/providers/amazon/aws/transfers/dynamodb_to_s3.py#L99
- https://github.com/aws-mwaa/upstream-to-airflow/blob/6d048c43f0753d96976f3c9e72262cfe3b27d052/providers/src/airflow/providers/amazon/aws/operators/emr.py#L642
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #45660:
URL: https://github.com/apache/airflow/pull/45660#discussion_r1917400019
##########
providers/src/airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -315,6 +318,9 @@ def submit_job(self, context: Context):
"schedulingPriorityOverride": self.scheduling_priority_override,
}
+ if self.submit_job_timeout:
+ args["timeout"] = {"attemptDurationSeconds": self.submit_job_timeout}
Review Comment:
Nice, very pretty now :slightly_smiling_face:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #45660:
URL: https://github.com/apache/airflow/pull/45660#discussion_r1917232670
##########
providers/src/airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -184,6 +185,7 @@ def __init__(
poll_interval: int = 30,
awslogs_enabled: bool = False,
awslogs_fetch_interval: timedelta = timedelta(seconds=30),
+ boto3_timeout: dict | None = None,
Review Comment:
I think I would prefer `submit_job_timeout` if you don't mind!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2599279469
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "nrobinson-intelycare (via GitHub)" <gi...@apache.org>.
nrobinson-intelycare commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2640163935
@eladkal @vincbeck Any plans for making a release soon?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2607522985
Usually releases are done every 2 weeks but @eladkal should be able to give you more details about the next wave.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "nrobinson-intelycare (via GitHub)" <gi...@apache.org>.
nrobinson-intelycare commented on code in PR #45660:
URL: https://github.com/apache/airflow/pull/45660#discussion_r1917216476
##########
providers/src/airflow/providers/amazon/aws/operators/batch.py:
##########
@@ -184,6 +185,7 @@ def __init__(
poll_interval: int = 30,
awslogs_enabled: bool = False,
awslogs_fetch_interval: timedelta = timedelta(seconds=30),
+ boto3_timeout: dict | None = None,
Review Comment:
I've named it `batch_execution_timeout` in the latest commit, but I can change it to `submit_job_timeout` if that's better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for timeout to BatchOperator [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #45660:
URL: https://github.com/apache/airflow/pull/45660#issuecomment-2594015899
> > > @o-nikolas @vincbeck @eladkal
> > > Anything I'm missing blocking this from being merged?
> >
> >
> > Just curious, have you tested it manually when `submit_job_timeout` is not provided? In that case `timeout` is passed to the the api `submit_job` with value `{"attemptDurationSeconds": None}`. I am wondering it Boto3 is okay with that
>
> I've changed it to only add `timeout` to the `args` dict if submit_job_timeout is not None:
>
> https://github.com/apache/airflow/pull/45660/files#diff-730ff9a9f5255ee440584231fc8733730c53a6a1a3fa3c2edd993c9d5157f219R322
I missed that! LGTM :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org