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/07/21 16:58:28 UTC
[GitHub] [airflow] pmalafosse opened a new pull request #17141: pass context to self.xcom_pull as it was missing
pmalafosse opened a new pull request #17141:
URL: https://github.com/apache/airflow/pull/17141
FYI @darwinyip
This is to fix a bug I found with context not being passed to xcom_pull:
```
Traceback (most recent call last):
File "/home/airflow/.local/bin/airflow", line 8, in <module>
sys.exit(main())
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/__main__.py", line 40, in main
args.func(args)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/cli/cli_parser.py", line 48, in command
return func(*args, **kwargs)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/utils/cli.py", line 91, in wrapper
return f(*args, **kwargs)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/cli/commands/task_command.py", line 393, in task_test
ti.run(ignore_task_deps=True, ignore_ti_state=True, test_mode=True)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/utils/session.py", line 70, in wrapper
return func(*args, session=session, **kwargs)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1432, in run
self._run_raw_task(
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/utils/session.py", line 67, in wrapper
return func(*args, **kwargs)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1157, in _run_raw_task
self._prepare_and_execute_task_with_callbacks(context, task)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1331, in _prepare_and_execute_task_with_callbacks
result = self._execute_task(context, task_copy)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1356, in _execute_task
result = task_copy.execute(context=context)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/utils/session.py", line 70, in wrapper
return func(*args, session=session, **kwargs)
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/providers/amazon/aws/operators/ecs.py", line 218, in execute
self._try_reattach_task()
File "/home/airflow/.local/lib/python3.8/site-packages/airflow/providers/amazon/aws/operators/ecs.py", line 309, in _try_reattach_task
previous_task_arn = self.xcom_pull(
TypeError: xcom_pull() missing 1 required positional argument: 'context'
```
I updated the unit tests and ran that new code locally to make sure it was working as expected.
@o-nikolas @kaxil
--
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] pmalafosse commented on pull request #17141: ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on pull request #17141:
URL: https://github.com/apache/airflow/pull/17141#issuecomment-884361263
> Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided). I'm curious how this got past manual testing though.
this happened because in my manual testing I was using a previous version I had locally before a refactor :
`context["ti"].xcom_pull(task_ids=..., key=...)` to
```self.xcom_pull(
context,
task_ids=...,
key=...,
)
```
--
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] pmalafosse edited a comment on pull request #17141: ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)
Posted by GitBox <gi...@apache.org>.
pmalafosse edited a comment on pull request #17141:
URL: https://github.com/apache/airflow/pull/17141#issuecomment-884361263
> Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided). I'm curious how this got past manual testing though.
this happened because in my manual testing I was using a previous version I had locally before a refactor :
`context["ti"].xcom_pull(task_ids=..., key=...)` to
```
self.xcom_pull(
context,
task_ids=...,
key=...,
)
```
--
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 #17141: ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17141:
URL: https://github.com/apache/airflow/pull/17141#issuecomment-884344398
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main 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 pull request #17141: ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17141:
URL: https://github.com/apache/airflow/pull/17141#issuecomment-884366770
> Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided).
It's not very well understood concept but unit tests do not replace manual tests, not even are a proof that things are right. They are there mostly to make sure that behaviour have not changed and is the same as before (so prevent regression). And having unit tests written at all (whether they are correct or not) makes it simply easier to fix things and re-run tests after we find and fix problems (and fix test afterwards :)).
--
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] o-nikolas commented on pull request #17141: ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #17141:
URL: https://github.com/apache/airflow/pull/17141#issuecomment-884382415
> > Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided).
>
> It's not very well understood concept but unit tests do not replace manual tests, not even are a proof that things are right. They are there mostly to make sure that behaviour have not changed and is the same as before (so prevent regression). And having unit tests written at all (whether they are correct or not) makes it simply easier to fix things and re-run tests after we find and fix problems (and fix test afterwards :)).
Totally agree, system/E2E tests are also key for catching these types of things (i.e. functional issues) rather than the quick regression testing that unit tests provide.
--
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 #17141: ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #17141:
URL: https://github.com/apache/airflow/pull/17141
--
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] pmalafosse edited a comment on pull request #17141: ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)
Posted by GitBox <gi...@apache.org>.
pmalafosse edited a comment on pull request #17141:
URL: https://github.com/apache/airflow/pull/17141#issuecomment-884361263
> Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided). I'm curious how this got past manual testing though.
this happened because in my manual testing I was using a previous version I had locally before a refactor :
`context["ti"].xcom_pull(task_ids=..., key=...)` to
```
self.xcom_pull(
context,
task_ids=...,
key=...,
)
```
I had to do this refactor to use mocking in the tests
--
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] kaxil commented on pull request #17141: ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #17141:
URL: https://github.com/apache/airflow/pull/17141#issuecomment-884370760
Thanks 👏 @pmalafosse
--
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