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/04/20 14:22:19 UTC
[GitHub] [airflow] ashb opened a new pull request, #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
ashb opened a new pull request, #23121:
URL: https://github.com/apache/airflow/pull/23121
There are a few ways we can get an exception before ever making it to user code, and if that happens _also_ warning about "this shouldn't happen" is obfuscating the original error.
Before:
```
[2022-04-20, 14:48:44 BST] 1103834 QueuedLocalWorker-3 {{airflow.models.taskinstance.TaskInstance taskinstance.py:1865}} WARNING - We expected to get frame set in local storage but it was not. Please report this as an issue with full logs at https://github.com/apache/airflow/issues/new
Traceback (most recent call last):
File "/home/ash/code/airflow/airflow/airflow/models/taskinstance.py", line 1442, in _run_raw_task
self._execute_task_with_callbacks(context, test_mode)
File "/home/ash/code/airflow/airflow/airflow/models/taskinstance.py", line 1546, in _execute_task_with_callbacks
task_orig = self.render_templates(context=context)
File "/home/ash/code/airflow/airflow/airflow/models/taskinstance.py", line 2210, in render_templates
rendered_task = self.task.render_template_fields(context)
File "/home/ash/code/airflow/airflow/airflow/models/mappedoperator.py", line 724, in render_template_fields
unmapped_task = self.unmap(unmap_kwargs=kwargs)
File "/home/ash/code/airflow/airflow/airflow/models/mappedoperator.py", line 510, in unmap
op = self.operator_class(**unmap_kwargs, _airflow_from_mapped=True)
File "/home/ash/code/airflow/airflow/airflow/models/baseoperator.py", line 390, in apply_defaults
result = func(self, **kwargs, default_args=default_args)
File "/home/ash/code/airflow/airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py", line 259, in __init__
self.name = self._set_name(name)
File "/home/ash/code/airflow/airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py", line 442, in _set_name
raise AirflowException("`name` is required unless `pod_template_file` or `full_pod_spec` is set")
airflow.exceptions.AirflowException: `name` is required unless `pod_template_file` or `full_pod_spec` is set
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/ash/code/airflow/airflow/airflow/models/taskinstance.py", line 1863, in get_truncated_error_traceback
execution_frame = _TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame
AttributeError: '_thread._local' object has no attribute 'frame'
[2022-04-20, 14:48:44 BST] 1103834 QueuedLocalWorker-3 {{airflow.models.taskinstance.TaskInstance taskinstance.py:1896}} ERROR - Task failed with exception
Traceback (most recent call last):
File "/home/ash/code/airflow/airflow/airflow/models/taskinstance.py", line 1442, in _run_raw_task
self._execute_task_with_callbacks(context, test_mode)
File "/home/ash/code/airflow/airflow/airflow/models/taskinstance.py", line 1546, in _execute_task_with_callbacks
task_orig = self.render_templates(context=context)
File "/home/ash/code/airflow/airflow/airflow/models/taskinstance.py", line 2210, in render_templates
rendered_task = self.task.render_template_fields(context)
File "/home/ash/code/airflow/airflow/airflow/models/mappedoperator.py", line 724, in render_template_fields
unmapped_task = self.unmap(unmap_kwargs=kwargs)
File "/home/ash/code/airflow/airflow/airflow/models/mappedoperator.py", line 510, in unmap
op = self.operator_class(**unmap_kwargs, _airflow_from_mapped=True)
File "/home/ash/code/airflow/airflow/airflow/models/baseoperator.py", line 390, in apply_defaults
result = func(self, **kwargs, default_args=default_args)
File "/home/ash/code/airflow/airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py", line 259, in __init__
self.name = self._set_name(name)
File "/home/ash/code/airflow/airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py", line 442, in _set_name
raise AirflowException("`name` is required unless `pod_template_file` or `full_pod_spec` is set")
airflow.exceptions.AirflowException: `name` is required unless `pod_template_file` or `full_pod_spec` is set
```
After we only see the error once, which is much better
<!--
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] ashb commented on a diff in pull request #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #23121:
URL: https://github.com/apache/airflow/pull/23121#discussion_r854336707
##########
airflow/models/taskinstance.py:
##########
@@ -1650,6 +1652,8 @@ def _execute_task(self, context, task_orig):
except: # noqa: E722
_TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame = currentframe()
raise
+ finally:
+ del _TASK_EXECUTION_FRAME_LOCAL_STORAGE.in_user_code
Review Comment:
Done
--
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 merged pull request #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
ashb merged PR #23121:
URL: https://github.com/apache/airflow/pull/23121
--
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 #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23121:
URL: https://github.com/apache/airflow/pull/23121#discussion_r854542060
##########
airflow/models/taskinstance.py:
##########
@@ -1845,33 +1838,21 @@ def _handle_reschedule(
session.commit()
self.log.info('Rescheduling task, marking task as UP_FOR_RESCHEDULE')
- def get_truncated_error_traceback(self, error: BaseException) -> Optional[TracebackType]:
+ @staticmethod
+ def get_truncated_error_traceback(error: BaseException, truncate_to: Callable) -> Optional[TracebackType]:
"""
- Returns truncated error traceback.
-
- This method returns traceback of the error truncated to the
- frame saved by earlier try/except along the way. If the frame
- is found, the traceback will be truncated to below the frame.
+ Truncates the traceback of an exception to the first frame called from within a given function
:param error: exception to get traceback from
- :return: traceback to print
+ :param truncate_to: Function to truncate TB to. Must have a ``__code__`` attribute
+
+ :meta private:
"""
tb = error.__traceback__
- try:
- execution_frame = _TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame
- except AttributeError:
- self.log.warning(
- "We expected to get frame set in local storage but it was not."
- " Please report this as an issue with full logs"
- " at https://github.com/apache/airflow/issues/new",
- exc_info=True,
- )
- return tb
- _TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame = None
+ code = truncate_to.__func__.__code__ # type: ignore[attr-defined]
Review Comment:
I like the idea, but seems that `__code__' is quite a big dict, which might be long to compare. Maybe instead of comparing __code__ we should use tuple of (filename, lineno, name) - which seems to be present in each stack frame and should be much faster to compare ?
--
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 #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23121:
URL: https://github.com/apache/airflow/pull/23121#issuecomment-1104181666
> @potiuk I think you must have introduced this issue when you refactored the code (not any blame here, just noting it).
Actually that is a good idea the "this should never happen" case was handled allowing to detect 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] ashb commented on pull request #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #23121:
URL: https://github.com/apache/airflow/pull/23121#issuecomment-1104182076
Wait this approach doesn't work..
But I think I have a way of greatly simplifying this and not even needing thread local storage!
--
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 #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23121:
URL: https://github.com/apache/airflow/pull/23121#issuecomment-1104201906
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] ashb commented on pull request #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #23121:
URL: https://github.com/apache/airflow/pull/23121#issuecomment-1104321573
PTAL @potiuk @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] jedcunningham commented on a diff in pull request #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #23121:
URL: https://github.com/apache/airflow/pull/23121#discussion_r854290356
##########
airflow/models/taskinstance.py:
##########
@@ -1650,6 +1652,8 @@ def _execute_task(self, context, task_orig):
except: # noqa: E722
_TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame = currentframe()
raise
+ finally:
+ del _TASK_EXECUTION_FRAME_LOCAL_STORAGE.in_user_code
Review Comment:
Should we set this to False instead of deleting it? Might not actually matter, but could let us simplify `get_truncated_error_traceback`?
--
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 #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23121:
URL: https://github.com/apache/airflow/pull/23121#discussion_r855103148
##########
airflow/models/taskinstance.py:
##########
@@ -1845,33 +1838,21 @@ def _handle_reschedule(
session.commit()
self.log.info('Rescheduling task, marking task as UP_FOR_RESCHEDULE')
- def get_truncated_error_traceback(self, error: BaseException) -> Optional[TracebackType]:
+ @staticmethod
+ def get_truncated_error_traceback(error: BaseException, truncate_to: Callable) -> Optional[TracebackType]:
"""
- Returns truncated error traceback.
-
- This method returns traceback of the error truncated to the
- frame saved by earlier try/except along the way. If the frame
- is found, the traceback will be truncated to below the frame.
+ Truncates the traceback of an exception to the first frame called from within a given function
:param error: exception to get traceback from
- :return: traceback to print
+ :param truncate_to: Function to truncate TB to. Must have a ``__code__`` attribute
+
+ :meta private:
"""
tb = error.__traceback__
- try:
- execution_frame = _TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame
- except AttributeError:
- self.log.warning(
- "We expected to get frame set in local storage but it was not."
- " Please report this as an issue with full logs"
- " at https://github.com/apache/airflow/issues/new",
- exc_info=True,
- )
- return tb
- _TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame = None
+ code = truncate_to.__func__.__code__ # type: ignore[attr-defined]
Review Comment:
Right!
--
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 #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23121:
URL: https://github.com/apache/airflow/pull/23121#issuecomment-1104183775
> But I think I have a way of greatly simplifying this and not even needing thread local storage!
:eyes:
--
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 diff in pull request #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #23121:
URL: https://github.com/apache/airflow/pull/23121#discussion_r854546694
##########
airflow/models/taskinstance.py:
##########
@@ -1845,33 +1838,21 @@ def _handle_reschedule(
session.commit()
self.log.info('Rescheduling task, marking task as UP_FOR_RESCHEDULE')
- def get_truncated_error_traceback(self, error: BaseException) -> Optional[TracebackType]:
+ @staticmethod
+ def get_truncated_error_traceback(error: BaseException, truncate_to: Callable) -> Optional[TracebackType]:
"""
- Returns truncated error traceback.
-
- This method returns traceback of the error truncated to the
- frame saved by earlier try/except along the way. If the frame
- is found, the traceback will be truncated to below the frame.
+ Truncates the traceback of an exception to the first frame called from within a given function
:param error: exception to get traceback from
- :return: traceback to print
+ :param truncate_to: Function to truncate TB to. Must have a ``__code__`` attribute
+
+ :meta private:
"""
tb = error.__traceback__
- try:
- execution_frame = _TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame
- except AttributeError:
- self.log.warning(
- "We expected to get frame set in local storage but it was not."
- " Please report this as an issue with full logs"
- " at https://github.com/apache/airflow/issues/new",
- exc_info=True,
- )
- return tb
- _TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame = None
+ code = truncate_to.__func__.__code__ # type: ignore[attr-defined]
Review Comment:
We are comparing with `is` -- so it is comparing is it the exact same object -- no `__eq__` function or equivalent is checked, so its as quick as it can be.
--
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 diff in pull request #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #23121:
URL: https://github.com/apache/airflow/pull/23121#discussion_r854291369
##########
airflow/models/taskinstance.py:
##########
@@ -1650,6 +1652,8 @@ def _execute_task(self, context, task_orig):
except: # noqa: E722
_TASK_EXECUTION_FRAME_LOCAL_STORAGE.frame = currentframe()
raise
+ finally:
+ del _TASK_EXECUTION_FRAME_LOCAL_STORAGE.in_user_code
Review Comment:
We could -- I'm not sure why I chose delete than set to False tbh.
--
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 pull request #23121: Don't try to truncate tracebacks in Tasks outside of "user" code
Posted by GitBox <gi...@apache.org>.
malthe commented on PR #23121:
URL: https://github.com/apache/airflow/pull/23121#issuecomment-1104152666
@potiuk I think you must have introduced this issue when you refactored the code (not any blame here, just noting 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