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/01/27 15:26:01 UTC
[GitHub] [airflow] Junnplus opened a new pull request #13929: Remove redundant session arg
Junnplus opened a new pull request #13929:
URL: https://github.com/apache/airflow/pull/13929
closes https://github.com/apache/airflow/issues/13797
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-782547826
[The Workflow run](https://github.com/apache/airflow/actions/runs/583343708) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-793408640
@ashb @kaxil We can merge this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-773998708
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 master 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Remove redundant session arg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-768486128
[The Workflow run](https://github.com/apache/airflow/actions/runs/515844283) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pysalt commented on a change in pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
pysalt commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r571798469
##########
File path: airflow/sentry.py
##########
@@ -149,14 +149,21 @@ def add_breadcrumbs(self, task_instance, session=None):
def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
+ session_args_idx = find_session_idx(func)
Review comment:
Is maybe need to change method annotation?
in airflow 2.0.0 this decorator wraps `_run_mini_scheduler_on_child_tasks` too. which is why the error actually occurred
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Remove redundant session arg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-768483429
[The Workflow run](https://github.com/apache/airflow/actions/runs/515773438) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-774256201
[The Workflow run](https://github.com/apache/airflow/actions/runs/540832426) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pysalt commented on a change in pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
pysalt commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r571798469
##########
File path: airflow/sentry.py
##########
@@ -149,14 +149,21 @@ def add_breadcrumbs(self, task_instance, session=None):
def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
+ session_args_idx = find_session_idx(func)
Review comment:
Is maybe need to change method annotation?
in airflow 2.0.0 this decorator wraps _run_mini_scheduler_on_child_tasks too. which is why the error actually occurred
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13929: Remove redundant session arg
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r570137851
##########
File path: airflow/sentry.py
##########
@@ -151,15 +151,15 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
with sentry_sdk.push_scope():
try:
- return func(task_instance, *args, session=session, **kwargs)
+ return func(task_instance, *args, **kwargs)
except Exception as e:
self.add_tagging(task_instance)
- self.add_breadcrumbs(task_instance, session=session)
+ self.add_breadcrumbs(task_instance)
Review comment:
@Junnplus Probably worth doing, yeah.
We already have https://github.com/apache/airflow/blob/49aa9aa2cd6e36ec211382d30a58e279755b0962/airflow/utils/session.py -- so perhaps that could be extracted somehow to not have duplicated logic?
##########
File path: airflow/sentry.py
##########
@@ -151,12 +151,19 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
+
+ try:
+ session_args_idx = find_session_idx(func)
+ session = kwargs.get('session', args[session_args_idx])
+ except Exception:
Review comment:
This exception is too broad
##########
File path: airflow/sentry.py
##########
@@ -151,12 +151,19 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
+
+ try:
+ session_args_idx = find_session_idx(func)
Review comment:
This should be called/"cached" outside the `wrapper` so that every call doesn't need to re-calculate this.
##########
File path: airflow/utils/session.py
##########
@@ -56,6 +51,18 @@ def provide_session(func: Callable[..., RT]) -> Callable[..., RT]:
# We don't need this anymore -- ensure we don't keep a reference to it by mistake
del func_params
Review comment:
```suggestion
```
Not needed anymore now its a separate function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus edited a comment on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
Junnplus edited a comment on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-774018890
@ashb Some new checks were not successful.
Seems `mock.create_autospec` has a problem when python version < 3.7.3, I can skip these test?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r570530609
##########
File path: airflow/sentry.py
##########
@@ -151,12 +151,19 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
+
+ try:
+ session_args_idx = find_session_idx(func)
+ session = kwargs.get('session', args[session_args_idx])
+ except Exception:
Review comment:
This exception is too broad
##########
File path: airflow/sentry.py
##########
@@ -151,12 +151,19 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
+
+ try:
+ session_args_idx = find_session_idx(func)
Review comment:
This should be called/"cached" outside the `wrapper` so that every call doesn't need to re-calculate this.
##########
File path: airflow/utils/session.py
##########
@@ -56,6 +51,18 @@ def provide_session(func: Callable[..., RT]) -> Callable[..., RT]:
# We don't need this anymore -- ensure we don't keep a reference to it by mistake
del func_params
Review comment:
```suggestion
```
Not needed anymore now its a separate function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-773680646
[The Workflow run](https://github.com/apache/airflow/actions/runs/538748666) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r570901120
##########
File path: airflow/sentry.py
##########
@@ -150,13 +150,21 @@ def add_breadcrumbs(self, task_instance, session=None):
def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
+ session_args_idx = find_session_idx(func)
+
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
+
+ try:
+ session = kwargs.get('session', args[session_args_idx])
+ except IndexError:
+ session = None
Review comment:
Is this the right behaviour, or is session a required for `add_breadcrumbs` to work?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-774044866
The new tests you added are failing though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-774018890
@ashb Some checks were not successful.
Seems `mock.create_autospec` has a problem when python version < 3.7.3, I can skip these test?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13929: Remove redundant session arg
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r568700061
##########
File path: airflow/sentry.py
##########
@@ -151,15 +151,15 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
with sentry_sdk.push_scope():
try:
- return func(task_instance, *args, session=session, **kwargs)
+ return func(task_instance, *args, **kwargs)
except Exception as e:
self.add_tagging(task_instance)
- self.add_breadcrumbs(task_instance, session=session)
+ self.add_breadcrumbs(task_instance)
Review comment:
This will make the add_breadcrumbs use a different session object -- that's not great.
I wonder if instead the place where session is passed as a positional argument should be converted to use kwargs instead?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-773771798
[The Workflow run](https://github.com/apache/airflow/actions/runs/539264212) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r570916639
##########
File path: airflow/sentry.py
##########
@@ -150,13 +150,21 @@ def add_breadcrumbs(self, task_instance, session=None):
def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
+ session_args_idx = find_session_idx(func)
+
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
+
+ try:
+ session = kwargs.get('session', args[session_args_idx])
+ except IndexError:
+ session = None
Review comment:
This behavior remains the same as before.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13929: Remove redundant session arg
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r568700061
##########
File path: airflow/sentry.py
##########
@@ -151,15 +151,15 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
with sentry_sdk.push_scope():
try:
- return func(task_instance, *args, session=session, **kwargs)
+ return func(task_instance, *args, **kwargs)
except Exception as e:
self.add_tagging(task_instance)
- self.add_breadcrumbs(task_instance, session=session)
+ self.add_breadcrumbs(task_instance)
Review comment:
This will make the add_breadcrumbs use a different session object -- that's not great.
I wonder if instead the place where session is passed as a positional argument should be converted to use kwargs instead?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r570901200
##########
File path: airflow/utils/session.py
##########
@@ -40,21 +40,26 @@ def create_session():
RT = TypeVar("RT") # pylint: disable=invalid-name
+def find_session_idx(func: Callable[..., RT]) -> int:
+ """Find session index in function call parameter."""
+ func_params = signature(func).parameters
+ try:
+ # func_params is an ordered dict -- this is the "recommended" way of getting the position
+ session_args_idx = tuple(func_params).index("session")
+ except ValueError:
+ raise ValueError(f"Function {func.__qualname__} has no `session` argument") from None
Review comment:
Nice!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-773821025
[The Workflow run](https://github.com/apache/airflow/actions/runs/539508714) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r570921929
##########
File path: airflow/sentry.py
##########
@@ -150,13 +150,21 @@ def add_breadcrumbs(self, task_instance, session=None):
def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
+ session_args_idx = find_session_idx(func)
+
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
+
+ try:
+ session = kwargs.get('session', args[session_args_idx])
+ except IndexError:
+ session = None
Review comment:
Fair, point.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13929: Remove redundant session arg
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r570137851
##########
File path: airflow/sentry.py
##########
@@ -151,15 +151,15 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
with sentry_sdk.push_scope():
try:
- return func(task_instance, *args, session=session, **kwargs)
+ return func(task_instance, *args, **kwargs)
except Exception as e:
self.add_tagging(task_instance)
- self.add_breadcrumbs(task_instance, session=session)
+ self.add_breadcrumbs(task_instance)
Review comment:
@Junnplus Probably worth doing, yeah.
We already have https://github.com/apache/airflow/blob/49aa9aa2cd6e36ec211382d30a58e279755b0962/airflow/utils/session.py -- so perhaps that could be extracted somehow to not have duplicated logic?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #13929: Fix error when running tasks with Sentry integration enabled.
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #13929:
URL: https://github.com/apache/airflow/pull/13929
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r579575284
##########
File path: airflow/sentry.py
##########
@@ -149,14 +149,21 @@ def add_breadcrumbs(self, task_instance, session=None):
def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
+ session_args_idx = find_session_idx(func)
Review comment:
> which is why the error actually occurred
This is not the root cause. Just wrap `TaskInstance._run_raw_task` will also happen.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-773680646
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13929: Remove redundant session arg
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#discussion_r569147451
##########
File path: airflow/sentry.py
##########
@@ -151,15 +151,15 @@ def enrich_errors(self, func):
"""Wrap TaskInstance._run_raw_task to support task specific tags and breadcrumbs."""
@wraps(func)
- def wrapper(task_instance, *args, session=None, **kwargs):
+ def wrapper(task_instance, *args, **kwargs):
# Wrapping the _run_raw_task function with push_scope to contain
# tags and breadcrumbs to a specific Task Instance
with sentry_sdk.push_scope():
try:
- return func(task_instance, *args, session=session, **kwargs)
+ return func(task_instance, *args, **kwargs)
except Exception as e:
self.add_tagging(task_instance)
- self.add_breadcrumbs(task_instance, session=session)
+ self.add_breadcrumbs(task_instance)
Review comment:
> I wonder if instead the place where session is passed as a positional argument should be converted to use kwargs instead?
This only solves the current problem, maybe i should parse session from positional/kwargs argument?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on pull request #13929: Parse session from positional/kwargs argument
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #13929:
URL: https://github.com/apache/airflow/pull/13929#issuecomment-774608013
@ashb I mark the 3.6 test as skipped. Please check again, thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org