You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "SamWheating (via GitHub)" <gi...@apache.org> on 2023/03/02 00:03:46 UTC
[GitHub] [airflow] SamWheating opened a new pull request, #29850: Use hybrid_property for ti.try_number to fix broken filter in in /taskinstance/list View
SamWheating opened a new pull request, #29850:
URL: https://github.com/apache/airflow/pull/29850
closes: https://github.com/apache/airflow/issues/29843
Because the column and column in the DB are both called `try_number`, flask-appbuilder ends up trying to build a filter using the `try_number` property, which doesn't work.
I think that we _could_ get around this by renaming the property, but that would be a huge refactor. Instead, we can just use the `@hybrid_property` decorator which will enable different behaviour at the class vs. instance-level.
([more on sqlalchemy hybrid properties here](https://docs.sqlalchemy.org/en/14/orm/extensions/hybrid.html))
Tested this in breeze and can confirm it works:
<img width="1882" alt="image" src="https://user-images.githubusercontent.com/16950874/222294346-4eb9d7f5-1efa-4771-a475-3db9927b42a5.png">
Will wait for the full tests to run before I claim that this doesn't break anything else 🤞
--
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] zachliu commented on pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "zachliu (via GitHub)" <gi...@apache.org>.
zachliu commented on PR #29850:
URL: https://github.com/apache/airflow/pull/29850#issuecomment-1472682116
this is not yet in 2.5.2 right? :thinking:
--
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] potiuk commented on pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29850:
URL: https://github.com/apache/airflow/pull/29850#issuecomment-1472706985
FYI @pierrejeambrun @ephraimbuddy - > looks like this one was marked to 2.5.2 but was not cherry-picked. I guess it was some race condition (merging it around the release cut time).
I moved it to 2.5.3
--
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] potiuk commented on pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29850:
URL: https://github.com/apache/airflow/pull/29850#issuecomment-1458980989
Rebased to account for the caching/pipx issue.
--
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] potiuk merged pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29850:
URL: https://github.com/apache/airflow/pull/29850
--
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 diff in pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29850:
URL: https://github.com/apache/airflow/pull/29850#discussion_r1122655003
##########
airflow/models/taskinstance.py:
##########
@@ -139,6 +139,10 @@
from airflow.models.operator import Operator
from airflow.utils.task_group import MappedTaskGroup, TaskGroup
+ hybrid_property = property
+else:
+ from sqlalchemy.ext.hybrid import hybrid_property
Review Comment:
Why is the if-else needed?
--
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 diff in pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29850:
URL: https://github.com/apache/airflow/pull/29850#discussion_r1124054180
##########
airflow/models/taskinstance.py:
##########
@@ -139,6 +139,10 @@
from airflow.models.operator import Operator
from airflow.utils.task_group import MappedTaskGroup, TaskGroup
+ hybrid_property = property
+else:
+ from sqlalchemy.ext.hybrid import hybrid_property
Review Comment:
Can you add a TODO comment referencing the issue? This makes it easier to review and potentially remove the workwround in the future.
--
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] potiuk commented on a diff in pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29850:
URL: https://github.com/apache/airflow/pull/29850#discussion_r1125554106
##########
airflow/models/taskinstance.py:
##########
@@ -139,6 +139,12 @@
from airflow.models.operator import Operator
from airflow.utils.task_group import MappedTaskGroup, TaskGroup
+ # This is a workaround because mypy doesn't work with hybrid_property
+ # TODO: remove this hack and move hybrid_property back to main import block
Review Comment:
I suggest to add link to the specific issue here. It will be easier for anyone in the future to see it the issue is solved.
--
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] SamWheating commented on a diff in pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "SamWheating (via GitHub)" <gi...@apache.org>.
SamWheating commented on code in PR #29850:
URL: https://github.com/apache/airflow/pull/29850#discussion_r1122665539
##########
airflow/models/taskinstance.py:
##########
@@ -139,6 +139,10 @@
from airflow.models.operator import Operator
from airflow.utils.task_group import MappedTaskGroup, TaskGroup
+ hybrid_property = property
+else:
+ from sqlalchemy.ext.hybrid import hybrid_property
Review Comment:
It's a workaround for mypy not understanding the type of hybrid properties - more info here:
https://github.com/python/mypy/issues/4430#issuecomment-781272415
--
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] potiuk commented on pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29850:
URL: https://github.com/apache/airflow/pull/29850#issuecomment-1472700284
Should be. Did you check release notes?
--
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] potiuk commented on a diff in pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29850:
URL: https://github.com/apache/airflow/pull/29850#discussion_r1125553823
##########
airflow/models/taskinstance.py:
##########
@@ -139,6 +139,12 @@
from airflow.models.operator import Operator
from airflow.utils.task_group import MappedTaskGroup, TaskGroup
+ # This is a workaround because mypy doesn't work with hybrid_property
+ # TODO: remove this hack and move hybrid_property back to main import block
Review Comment:
```suggestion
# TODO: remove this hack and move hybrid_property back to main import block
# See https://github.com/python/mypy/issues/4430
```
--
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] potiuk commented on a diff in pull request #29850: Fixing broken filter in in /taskinstance/list view
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29850:
URL: https://github.com/apache/airflow/pull/29850#discussion_r1125556999
##########
airflow/models/taskinstance.py:
##########
@@ -139,6 +139,12 @@
from airflow.models.operator import Operator
from airflow.utils.task_group import MappedTaskGroup, TaskGroup
+ # This is a workaround because mypy doesn't work with hybrid_property
+ # TODO: remove this hack and move hybrid_property back to main import block
Review Comment:
(that is if I found the right issue of course)
--
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