You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "KoviAnusha (via GitHub)" <gi...@apache.org> on 2025/10/29 04:15:46 UTC
[PR] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
KoviAnusha opened a new pull request, #57465:
URL: https://github.com/apache/airflow/pull/57465
<!--
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 [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2476528892
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/dag_runs.py:
##########
@@ -48,7 +48,7 @@
status.HTTP_400_BAD_REQUEST: {"description": "DAG has import errors and cannot be triggered"},
status.HTTP_404_NOT_FOUND: {"description": "DAG not found for the given dag_id"},
status.HTTP_409_CONFLICT: {"description": "DAG Run already exists for the given dag_id"},
- status.HTTP_422_UNPROCESSABLE_ENTITY: {"description": "Invalid payload"},
+ status.HTTP_422_UNPROCESSABLE_CONTENT: {"description": "Invalid payload"},
Review Comment:
We’ll need to bump Starlette if we do this; currently we only require >=0.45.0, but HTTP_422_UNPROCESSABLE_CONTENT was added in 0.48.0.
https://github.com/Kludex/starlette/commit/40a81479c7146bab21cc58ccb92b017cf177a077
https://github.com/apache/airflow/blob/c3a0651b0c3739f5a842b0f49e1a11a0a385e862/airflow-core/pyproject.toml#L86
--
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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2471864032
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py:
##########
@@ -86,9 +86,9 @@ def get_asset_event_by_asset_name_uri(
if name and uri:
where_clause = and_(AssetModel.name == name, AssetModel.uri == uri)
elif uri:
- where_clause = and_(AssetModel.uri == uri, AssetModel.active.has())
+ where_clause = and_(AssetModel.uri == uri, AssetModel.active.has()) # type: ignore[attr-defined]
Review Comment:
Aren’t these just standard SQLAlchemy things? We shouldn’t have any errors here…
--
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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "KoviAnusha (via GitHub)" <gi...@apache.org>.
KoviAnusha commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2476323113
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py:
##########
@@ -86,9 +86,9 @@ def get_asset_event_by_asset_name_uri(
if name and uri:
where_clause = and_(AssetModel.name == name, AssetModel.uri == uri)
elif uri:
- where_clause = and_(AssetModel.uri == uri, AssetModel.active.has())
+ where_clause = and_(AssetModel.uri == uri, AssetModel.active.has()) # type: ignore[attr-defined]
Review Comment:
Yes, you are right. This is unnecessary. Thanks for pointing that out. Removing it.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -447,7 +446,7 @@ def _create_ti_state_update_query_and_update_state(
session: SessionDep,
dag_bag: DagBagDep,
dag_id: str,
-) -> tuple[Update, TaskInstanceState]:
+) -> tuple[Update, str]:
Review Comment:
Thank you. I will work on 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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2476337767
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py:
##########
@@ -65,7 +65,7 @@ def _get_asset_events_through_sql_clauses(
source_task_id=event.source_task_id,
source_dag_id=event.source_dag_id,
source_run_id=event.source_run_id,
- source_map_index=event.source_map_index,
+ source_map_index=event.source_map_index if event.source_map_index is not None else -1,
Review Comment:
Hmm, I think we should fix AssetEventResponse’s annotation instead. This value *should* be None, not -1, if the event does not come from a task instance (but created externally such as from an asset watcher).
--
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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2476309911
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -447,7 +446,7 @@ def _create_ti_state_update_query_and_update_state(
session: SessionDep,
dag_bag: DagBagDep,
dag_id: str,
-) -> tuple[Update, TaskInstanceState]:
+) -> tuple[Update, str]:
Review Comment:
PR has been 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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "KoviAnusha (via GitHub)" <gi...@apache.org>.
KoviAnusha commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2476457096
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py:
##########
@@ -65,7 +65,7 @@ def _get_asset_events_through_sql_clauses(
source_task_id=event.source_task_id,
source_dag_id=event.source_dag_id,
source_run_id=event.source_run_id,
- source_map_index=event.source_map_index,
+ source_map_index=event.source_map_index if event.source_map_index is not None else -1,
Review Comment:
Got it.
--
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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2471914411
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -447,7 +446,7 @@ def _create_ti_state_update_query_and_update_state(
session: SessionDep,
dag_bag: DagBagDep,
dag_id: str,
-) -> tuple[Update, TaskInstanceState]:
+) -> tuple[Update, str]:
Review Comment:
https://github.com/apache/airflow/pull/57468
--
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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #57465:
URL: https://github.com/apache/airflow/pull/57465
--
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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2476528892
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/dag_runs.py:
##########
@@ -48,7 +48,7 @@
status.HTTP_400_BAD_REQUEST: {"description": "DAG has import errors and cannot be triggered"},
status.HTTP_404_NOT_FOUND: {"description": "DAG not found for the given dag_id"},
status.HTTP_409_CONFLICT: {"description": "DAG Run already exists for the given dag_id"},
- status.HTTP_422_UNPROCESSABLE_ENTITY: {"description": "Invalid payload"},
+ status.HTTP_422_UNPROCESSABLE_CONTENT: {"description": "Invalid payload"},
Review Comment:
We’ll need to bump Starlette if we do this; currently we only require >=0.45.0, but HTTP_422_UNPROCESSABLE_CONTENT was added in 0.48.0.
https://github.com/Kludex/starlette/commit/40a81479c7146bab21cc58ccb92b017cf177a077
https://github.com/apache/airflow/blob/c3a0651b0c3739f5a842b0f49e1a11a0a385e862/airflow-core/pyproject.toml#L86
--
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] Fix MyPy type errors in api_fastapi/execution_api/ for Sqlalchemy 2 migration [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #57465:
URL: https://github.com/apache/airflow/pull/57465#discussion_r2471872838
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -447,7 +446,7 @@ def _create_ti_state_update_query_and_update_state(
session: SessionDep,
dag_bag: DagBagDep,
dag_id: str,
-) -> tuple[Update, TaskInstanceState]:
+) -> tuple[Update, str]:
Review Comment:
Hmm, I think the return type is the correct here, and we should keep it. The `updated_state` argument here is actually meaningless and should be removed. The input argument in `ti_update_state` is also meaningless. I’ll submit a separate PR to change it to make sense; this PR can be rebased once that’s merged into main.
--
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