You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/02/12 19:43:24 UTC
[GitHub] [airflow] iameugenejo opened a new pull request #14212: fix type mismatch of max_retry_delay
iameugenejo opened a new pull request #14212:
URL: https://github.com/apache/airflow/pull/14212
closes: #13086
Sorry about no test. I don't have in-depth knowledge of this project to come up one.
I'm just trying to fix what's not tested in the first place and broken.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb closed pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
ashb closed pull request #14212:
URL: https://github.com/apache/airflow/pull/14212
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#discussion_r577478212
##########
File path: airflow/models/taskinstance.py
##########
@@ -927,7 +927,7 @@ def next_retry_datetime(self):
delay_backoff_in_seconds = min(modded_hash, timedelta.max.total_seconds() - 1)
delay = timedelta(seconds=delay_backoff_in_seconds)
if self.task.max_retry_delay:
- delay = min(self.task.max_retry_delay, delay)
+ delay = timedelta(seconds=min(self.task.max_retry_delay, delay.total_seconds()))
Review comment:
This change isn't needed -- task.max_retry_delay should be a timedelta)
We should check this in the ctor of BaseOperator, and if what is passed is not a timedleta then do:
```python
if not isinstance(max_retry_delay):
max_retry_delay = timedelta(seconds=max_retry_delay)
```
etc.
Could you add a test to tests/models/test_baseoperator.py that checks this upgrade behaviour?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#issuecomment-778412401
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#issuecomment-780441110
> I honestly do not know where this `max_retry_delay` in `timedelta` type gets reassigned as a `float`. so please close this PR if someone finds the code that does that and fix it there as a permanent solution
Max retry delay is taken from what ever the DAG author puts in their ctor args
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iameugenejo edited a comment on pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
iameugenejo edited a comment on pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#issuecomment-782971311
> > I honestly do not know where this `max_retry_delay` in `timedelta` type gets reassigned as a `float`. so please close this PR if someone finds the code that does that and fix it there as a permanent solution
>
> Max retry delay is taken from what ever the DAG author puts in their ctor args
That's what I thought, and I have every dag with the max_retry_delay set with timedelta value, and the scheduler still fails to start someteimes because this value gets converted to float at some point. My guess is that it must be happening for dynamic dags. I currently have no time to make this change formalized with testing or find the cause of such conversion. I'll make the deployment of my environment with this patch locally until someone else has time to debug and find the real cause of this. You can close this PR if you'd like.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iameugenejo commented on pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
iameugenejo commented on pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#issuecomment-782971311
> > I honestly do not know where this `max_retry_delay` in `timedelta` type gets reassigned as a `float`. so please close this PR if someone finds the code that does that and fix it there as a permanent solution
>
> Max retry delay is taken from what ever the DAG author puts in their ctor args
That's what I thought, and I have every dag with the max_retry_delay set with timedelta value, and the scheduler still fails to start because this value gets converted to float at some point sometimes (not everytime).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iameugenejo edited a comment on pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
iameugenejo edited a comment on pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#issuecomment-782971311
> > I honestly do not know where this `max_retry_delay` in `timedelta` type gets reassigned as a `float`. so please close this PR if someone finds the code that does that and fix it there as a permanent solution
>
> Max retry delay is taken from what ever the DAG author puts in their ctor args
That's what I thought, and I have every dag with the max_retry_delay set with timedelta value, and the scheduler still fails to start sometimes because this value gets converted to float at some point. My guess is that it must be happening for dynamic dags. I currently have no time to make this change formalized with testing or find the cause of such conversion. I'll make the deployment of my environment with this patch locally until someone else has time to debug and find the real cause of this. You can close this PR if you'd like.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] iameugenejo commented on pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
iameugenejo commented on pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#issuecomment-778418211
I honestly do not know where this `max_retry_delay` in `timedelta` type gets reassigned as a `float`. so please close this PR if someone finds the code that does that and fix it there as a permanent solution
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#issuecomment-786619637
Closed in favour of https://github.com/apache/airflow/pull/14436
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14212: fix type mismatch of max_retry_delay
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14212:
URL: https://github.com/apache/airflow/pull/14212#issuecomment-778426512
[The Workflow run](https://github.com/apache/airflow/actions/runs/562042431) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org