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/12/09 12:03:01 UTC
[GitHub] [airflow] uranusjr opened a new pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
uranusjr opened a new pull request #20163:
URL: https://github.com/apache/airflow/pull/20163
Some more typing fixups found when implementing another feature, submitted separately.
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#discussion_r766145238
##########
File path: airflow/models/dagrun.py
##########
@@ -398,7 +398,9 @@ def generate_run_id(run_type: DagRunType, execution_date: datetime) -> str:
@provide_session
def get_task_instances(
- self, state: Optional[Iterable[TaskInstanceState]] = None, session=None
+ self,
+ state: Optional[Iterable[Optional[TaskInstanceState]]] = None,
+ session: Session = NEW_SESSION,
Review comment:
Submitted as #20181.
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#discussion_r766136225
##########
File path: airflow/models/dagrun.py
##########
@@ -398,7 +398,9 @@ def generate_run_id(run_type: DagRunType, execution_date: datetime) -> str:
@provide_session
def get_task_instances(
- self, state: Optional[Iterable[TaskInstanceState]] = None, session=None
+ self,
+ state: Optional[Iterable[Optional[TaskInstanceState]]] = None,
Review comment:
I suspect subclassing from `str` is the problem here (but we probably can’t fix that without breaking a ton of things).
--
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
[GitHub] [airflow] kaxil commented on a change in pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#discussion_r765765579
##########
File path: airflow/models/dagrun.py
##########
@@ -398,7 +398,9 @@ def generate_run_id(run_type: DagRunType, execution_date: datetime) -> str:
@provide_session
def get_task_instances(
- self, state: Optional[Iterable[TaskInstanceState]] = None, session=None
+ self,
+ state: Optional[Iterable[Optional[TaskInstanceState]]] = None,
+ session: Session = NEW_SESSION,
Review comment:
In a separate PR or this (your choice) - can you add a small section in https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices to let devs know when they should use `NEW_SESSION` vs `Session` for type hints
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#discussion_r765813838
##########
File path: airflow/utils/db.py
##########
@@ -882,12 +883,12 @@ def _move_dangling_task_data_to_new_table(session, source_table: "Table", target
_move_dangling_table(session, source_table, target_table_name, where_clause)
-def check_task_tables_without_matching_dagruns(session) -> Iterable[str]:
+def check_task_tables_without_matching_dagruns(session: Session) -> Iterable[str]:
import sqlalchemy.schema
from sqlalchemy import and_, outerjoin
metadata = sqlalchemy.schema.MetaData(session.bind)
- models_to_dagrun = [TaskInstance, TaskReschedule]
+ models_to_dagrun: List[Any] = [TaskInstance, TaskReschedule]
Review comment:
👍🏻 Also, as this isn't called anywhere from outside of this module it doesn't impact the typing of the rest of the code base.
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#issuecomment-989832607
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#discussion_r765725893
##########
File path: airflow/utils/db.py
##########
@@ -882,12 +883,12 @@ def _move_dangling_task_data_to_new_table(session, source_table: "Table", target
_move_dangling_table(session, source_table, target_table_name, where_clause)
-def check_task_tables_without_matching_dagruns(session) -> Iterable[str]:
+def check_task_tables_without_matching_dagruns(session: Session) -> Iterable[str]:
import sqlalchemy.schema
from sqlalchemy import and_, outerjoin
metadata = sqlalchemy.schema.MetaData(session.bind)
- models_to_dagrun = [TaskInstance, TaskReschedule]
+ models_to_dagrun: List[Any] = [TaskInstance, TaskReschedule]
Review comment:
Technically we can type this (what we want is the `__tablename__` attribute from SQLAlchemy), but SQLAlchemy is so dynamic, adding accurate type here does not really help much, so I don’t really want to bother.
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#issuecomment-989875741
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] uranusjr merged pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #20163:
URL: https://github.com/apache/airflow/pull/20163
--
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
[GitHub] [airflow] kaxil closed pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
kaxil closed pull request #20163:
URL: https://github.com/apache/airflow/pull/20163
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#discussion_r765811262
##########
File path: airflow/models/dagrun.py
##########
@@ -398,7 +398,9 @@ def generate_run_id(run_type: DagRunType, execution_date: datetime) -> str:
@provide_session
def get_task_instances(
- self, state: Optional[Iterable[TaskInstanceState]] = None, session=None
+ self,
+ state: Optional[Iterable[Optional[TaskInstanceState]]] = None,
Review comment:
Isn't `None` a valid TaskInstanceState? https://github.com/apache/airflow/blob/820e836c4a2e45239279d4d71e1db9434022fec5/airflow/utils/state.py#L35
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #20163: Mypy fixes to DagRun, TaskInstance, and db utils
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20163:
URL: https://github.com/apache/airflow/pull/20163#discussion_r765725893
##########
File path: airflow/utils/db.py
##########
@@ -882,12 +883,12 @@ def _move_dangling_task_data_to_new_table(session, source_table: "Table", target
_move_dangling_table(session, source_table, target_table_name, where_clause)
-def check_task_tables_without_matching_dagruns(session) -> Iterable[str]:
+def check_task_tables_without_matching_dagruns(session: Session) -> Iterable[str]:
import sqlalchemy.schema
from sqlalchemy import and_, outerjoin
metadata = sqlalchemy.schema.MetaData(session.bind)
- models_to_dagrun = [TaskInstance, TaskReschedule]
+ models_to_dagrun: List[Any] = [TaskInstance, TaskReschedule]
Review comment:
Technically we can type this (what we want is the `__tablename__` attribute from SQLAlchemy), but SQLAlchemy is so dynamic it does not really help much, so I don’t really want to bother.
--
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