-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
More typing in SchedulerJob and TaskInstance #24912
Conversation
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.
These are the only runtime changes in this PR, and even then they should be functionally identical.
TI.dag_id == dag_id, TI.state == TaskInstanceState.SCHEDULED | ||
).update({TI.state: TaskInstanceState.FAILED}, synchronize_session='fetch') |
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.
Technically just a typing change, but we were using the wrong one.
@@ -790,7 +794,7 @@ def _update_dag_run_state_for_paused_dags(self): | |||
dag = SerializedDagModel.get_dag(dag_id) | |||
if dag is None: | |||
continue | |||
dag_runs = DagRun.find(dag_id=dag_id, state=State.RUNNING) | |||
dag_runs = DagRun.find(dag_id=dag_id, state=DagRunState.RUNNING) |
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.
Also here, using the wrong one.
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
airflow/models/taskinstance.py
Outdated
@@ -197,7 +199,7 @@ def clear_task_instances( | |||
if dag and dag.has_task(task_id): | |||
task = dag.get_task(task_id) | |||
ti.refresh_from_task(task) | |||
task_retries = task.retries | |||
task_retries = task.retries or 0 |
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 also had to add this, as task.retries
is Optional[int]
. Not sure it would have worked if it was None, but I'll look closer tomorrow.
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.
It wouldn’t work, and honestly I don’t even know what None
means here. I think this is exposing a more fundamental bug…
I just pushed a commit to remove unnecessary |
Conflicts need to be resolved. |
I noticed these were missing some typing. This doesn't bring us to complete coverage in TaskInstance, but gets all the easy ones out of the way.