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 2022/03/04 12:55:07 UTC
[GitHub] [airflow] potiuk opened a new pull request #21993: Change the storage of frame to use threadLocal rather than Dict
potiuk opened a new pull request #21993:
URL: https://github.com/apache/airflow/pull/21993
There is a very probable WeakKeyDict bug in Python standard
library (to be confirmed and investigated further) that
manifests itself in a very rare failure of the
test_stacktrace_on_failure_starts_with_task_execute_method
This turned out to be related to an unexpected behaviour
(and most likely a bug - to be confirmed) of WeakKeyDict
when you have potentially two different objects with the
same `equals` and `hash` values added to the same
WeakKeyDict as keys.
More info on similar report (but raised for a bit different
reason) bug in Python can be found here:
https://bugs.python.org/issue44140
While we are still investigating the root cause and possibly
submit a bug to Python, this PR changes the mechanism
to store the frame in a Thread Local variable rather than
WeakRefDict.
<!--
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 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/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] malthe commented on a change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
malthe commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819578828
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,14 +1745,23 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
tb = error.__traceback__
- while tb is not None:
- if tb.tb_frame is execution_frame:
+ try:
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
+ while tb is not None:
+ if tb.tb_frame is execution_frame:
+ tb = tb.tb_next
+ break
tb = tb.tb_next
- break
- tb = tb.tb_next
- tb = tb or error.__traceback__
+ tb = tb or error.__traceback__
+ except AttributeError:
Review comment:
It feels perhaps like nitpicking, but the stuff we're worried about here is getting the frame and making sure that it's set.
This code doesn't actually raise an error if it is set but is `None` and the try/except could and should be limited to the code that we believe could lead to an error.
The `while` loop here should never cause an error.
--
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 change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819564067
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,7 +1745,8 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
Review comment:
Aren't you afraid of some edge cases here @malthe ? I think Iprefer to have a logical error handled nicely here as this is not a critical stuff (it will work just fine). Maybe we should instead issue WARNING if it is not set 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
[GitHub] [airflow] malthe commented on a change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
malthe commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819548088
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,7 +1745,8 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
Review comment:
I think `del` is nicer because then it will fail if there is a logical error in the code.
--
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 #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#issuecomment-1059365140
Looks like a docs-build inventory fetching problem only. Close/reopen to rebuild to be 100% sure.
--
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] malthe commented on a change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
malthe commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819569448
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,7 +1745,8 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
Review comment:
Yes we can do that:
```python
if _LAST_FRAME_LOCAL_STORAGE.__dict__.pop("frame", None) is None:
warn(...)
```
--
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 change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819571824
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,7 +1745,8 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
Review comment:
Yeah. I optimized the code a little bit and did it a bit differently (more info). One minute.
--
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 closed pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #21993:
URL: https://github.com/apache/airflow/pull/21993
--
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 #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#issuecomment-1059263633
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] potiuk commented on a change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819572984
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,7 +1745,8 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
Review comment:
See now @malthe
--
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 change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819593533
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,14 +1745,23 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
tb = error.__traceback__
- while tb is not None:
- if tb.tb_frame is execution_frame:
+ try:
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
+ while tb is not None:
+ if tb.tb_frame is execution_frame:
+ tb = tb.tb_next
+ break
tb = tb.tb_next
- break
- tb = tb.tb_next
- tb = tb or error.__traceback__
+ tb = tb or error.__traceback__
+ except AttributeError:
Review comment:
Yeah. Crossed my mind too (exact same line of thoughts :)). Will fix 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
[GitHub] [airflow] potiuk closed pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #21993:
URL: https://github.com/apache/airflow/pull/21993
--
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 change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819664282
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,14 +1745,23 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
tb = error.__traceback__
- while tb is not None:
- if tb.tb_frame is execution_frame:
+ try:
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
+ while tb is not None:
+ if tb.tb_frame is execution_frame:
+ tb = tb.tb_next
+ break
tb = tb.tb_next
- break
- tb = tb.tb_next
- tb = tb or error.__traceback__
+ tb = tb or error.__traceback__
+ except AttributeError:
Review comment:
Fixed
--
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 #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #21993:
URL: https://github.com/apache/airflow/pull/21993
--
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] malthe commented on a change in pull request #21993: Change the storage of frame to use threadLocal rather than Dict
Posted by GitBox <gi...@apache.org>.
malthe commented on a change in pull request #21993:
URL: https://github.com/apache/airflow/pull/21993#discussion_r819569448
##########
File path: airflow/models/taskinstance.py
##########
@@ -1746,7 +1745,8 @@ def handle_failure(
if error:
if isinstance(error, BaseException):
- execution_frame = _EXECUTION_FRAME_MAPPING.get(self.task)
+ execution_frame = _LAST_FRAME_LOCAL_STORAGE.frame
+ _LAST_FRAME_LOCAL_STORAGE.frame = None
Review comment:
Yes we can do that:
```python
execution_frame = _LAST_FRAME_LOCAL_STORAGE.__dict__.pop("frame", None)
if execution_frame is None:
warn(...)
```
--
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