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/11/03 06:00:15 UTC
[GitHub] [airflow] KennethanCeyer opened a new pull request #19378: Change logLevel as debug for XCOM returned value message
KennethanCeyer opened a new pull request #19378:
URL: https://github.com/apache/airflow/pull/19378
<!--
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 an 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/
-->
related: #18264
---
The current Airflow version (2.x.x) supports [Custom XCom Backends](https://www.astronomer.io/guides/custom-xcom-backends). and It means that the data from XCom could be larger than we think. (e.g. over `100GB`)
However, `PythonOperator` and `LevelDBOperator` will try to show all of those data from XCom as a log message. It will break the web page (due to browser memory issue or rendering speed issue). and It will make it difficult to see the other log message from the task.
This PR suggests changing the logLevel `info` to `debug`, Because XCom returned value can be treated `debug-purpose`
**^ 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] KennethanCeyer commented on a change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r743517890
##########
File path: airflow/operators/python.py
##########
@@ -123,6 +123,9 @@ def my_python_callable(**kwargs):
:param templates_exts: a list of file extensions to resolve while
processing templated fields, for examples ``['.sql', '.hql']``
:type templates_exts: list[str]
+ :param show_return_value_in_logs: a bool value whether to show return_value
Review comment:
I just edited the docstring explanation, but
I will think about it a little more and improve it soon.
##########
File path: airflow/operators/python.py
##########
@@ -123,6 +123,9 @@ def my_python_callable(**kwargs):
:param templates_exts: a list of file extensions to resolve while
processing templated fields, for examples ``['.sql', '.hql']``
:type templates_exts: list[str]
+ :param show_return_value_in_logs: a bool value whether to show return_value
Review comment:
I just edited the docstring explanation, but
I will think about it a little more and improve it soon.
--
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] KennethanCeyer commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-966898703
One quick question!
If the user uses `DecoratedOperator` as shown below through Task Flow API,
there seems to be no way to block XCom return value logs.
```python
@task
def my_task(): # Guess it is `_PythonDecoratedOperator`
# ... process
return large_data
with DAG(...) as dag:
data = my_task() # It will make a huge log message on Airflow UI
my_second_task(data)
```
so, I think additional work will be required in a separate PR for TaskFlow API users.
What do you think?
https://github.com/apache/airflow/blob/a9772cf287111a63eac8c2deb1190f7054d7580f/airflow/decorators/python.py#L49-L59
--
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 edited a comment on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-967047503
Yes we should do something for this but separately. Maybe something like (just a random idea)
```python
@task(..., log_return_value="DEBUG")
def my_task():
...
return large_data
```
--
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 #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r741668839
##########
File path: airflow/operators/python.py
##########
@@ -171,7 +171,7 @@ def execute(self, context: Dict):
self.op_kwargs = determine_kwargs(self.python_callable, self.op_args, context)
return_value = self.execute_callable()
- self.log.info("Done. Returned value was: %s", return_value)
+ self.log.debug("Done. Returned value was: %s", return_value)
Review comment:
I do not think, changing the log level is good solution. There are many use cases where peopel might rely on the return value appear in logs.
I think better one might be to add optional parameter (default to True) `show_return_value_in_logs` that could be set to False tu suppress printing the value to the logs - with appropriate explanation of the "big" values.
##########
File path: airflow/providers/google/leveldb/operators/leveldb.py
##########
@@ -92,7 +92,7 @@ def execute(self, context) -> Optional[str]:
keys=self.keys,
values=self.values,
)
- self.log.info("Done. Returned value was: %s", str(value))
+ self.log.debug("Done. Returned value was: %s", str(value))
leveldb_hook.close_conn()
value = value if value is None else value.decode()
Review comment:
Here I also think we need to do more: str(value) will actually perform string conversion no matter what is the log value (and it is not needed anyway because %s will do it anyway. So here both 'if' and removing str is 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] potiuk commented on a change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r741669688
##########
File path: airflow/providers/google/leveldb/operators/leveldb.py
##########
@@ -92,7 +92,7 @@ def execute(self, context) -> Optional[str]:
keys=self.keys,
values=self.values,
)
- self.log.info("Done. Returned value was: %s", str(value))
+ self.log.debug("Done. Returned value was: %s", str(value))
leveldb_hook.close_conn()
value = value if value is None else value.decode()
Review comment:
Here I also think we need to do more: str(value) will actually perform string conversion no matter what is the log value (and it is not needed anyway because %s will do it anyway. So here both 'if' and removing str is 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] potiuk commented on a change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r741668839
##########
File path: airflow/operators/python.py
##########
@@ -171,7 +171,7 @@ def execute(self, context: Dict):
self.op_kwargs = determine_kwargs(self.python_callable, self.op_args, context)
return_value = self.execute_callable()
- self.log.info("Done. Returned value was: %s", return_value)
+ self.log.debug("Done. Returned value was: %s", return_value)
Review comment:
I do not think, changing the log level is good solution. There are many use cases where peopel might rely on the return value appear in logs.
I think better one might be to add optional parameter (default to True) `show_return_value_in_logs` that could be set to False tu suppress printing the value to the logs - with appropriate explanation of the "big" values.
##########
File path: airflow/providers/google/leveldb/operators/leveldb.py
##########
@@ -92,7 +92,7 @@ def execute(self, context) -> Optional[str]:
keys=self.keys,
values=self.values,
)
- self.log.info("Done. Returned value was: %s", str(value))
+ self.log.debug("Done. Returned value was: %s", str(value))
leveldb_hook.close_conn()
value = value if value is None else value.decode()
Review comment:
Here I also think we need to do more: str(value) will actually perform string conversion no matter what is the log value (and it is not needed anyway because %s will do it anyway. So here both 'if' and removing str is 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] potiuk commented on a change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r743454561
##########
File path: airflow/operators/python.py
##########
@@ -123,6 +123,9 @@ def my_python_callable(**kwargs):
:param templates_exts: a list of file extensions to resolve while
processing templated fields, for examples ``['.sql', '.hql']``
:type templates_exts: list[str]
+ :param show_return_value_in_logs: a bool value whether to show return_value
Review comment:
I think it would be great to explain when you would like to set it to "false" (i.e. explain why we have this parameter).
--
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] KennethanCeyer commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-967086764
@uranusjr
I see.
For that work, I will proceed by making another PR 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 change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r743454561
##########
File path: airflow/operators/python.py
##########
@@ -123,6 +123,9 @@ def my_python_callable(**kwargs):
:param templates_exts: a list of file extensions to resolve while
processing templated fields, for examples ``['.sql', '.hql']``
:type templates_exts: list[str]
+ :param show_return_value_in_logs: a bool value whether to show return_value
Review comment:
I think it would be great to explain when you would like to set it to "false" (i.e. explain why we have this parameter).
--
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] KennethanCeyer edited a comment on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer edited a comment on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-966898703
One quick question!
If the user uses `DecoratedOperator` as shown below through Task Flow API,
there seems to be no way to block XCom return value logs.
```python
@task
def my_task(): # Guess it is `_PythonDecoratedOperator`
# ... process
return large_data
with DAG(...) as dag:
data = my_task() # It will make a huge log messages on Airflow UI
my_second_task(data)
```
so, I think additional work will be required in a separate PR for TaskFlow API users.
What do you think?
Below are the locations that appear to require further work.
https://github.com/apache/airflow/blob/a9772cf287111a63eac8c2deb1190f7054d7580f/airflow/decorators/python.py#L49-L59
--
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 pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-972504634
I split the task API change into another issue (referenced above)
--
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] boring-cyborg[bot] commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958673519
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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 #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r743454561
##########
File path: airflow/operators/python.py
##########
@@ -123,6 +123,9 @@ def my_python_callable(**kwargs):
:param templates_exts: a list of file extensions to resolve while
processing templated fields, for examples ``['.sql', '.hql']``
:type templates_exts: list[str]
+ :param show_return_value_in_logs: a bool value whether to show return_value
Review comment:
I think it would be great to explain when you would like to set it to "false" (i.e. explain why we have this parameter).
--
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] KennethanCeyer commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958731328
--
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] boring-cyborg[bot] commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958673519
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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 #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r741668839
##########
File path: airflow/operators/python.py
##########
@@ -171,7 +171,7 @@ def execute(self, context: Dict):
self.op_kwargs = determine_kwargs(self.python_callable, self.op_args, context)
return_value = self.execute_callable()
- self.log.info("Done. Returned value was: %s", return_value)
+ self.log.debug("Done. Returned value was: %s", return_value)
Review comment:
I do not think, changing the log level is good solution. There are many use cases where peopel might rely on the return value appear in logs.
I think better one might be to add optional parameter (default to True) `show_return_value_in_logs` that could be set to False tu suppress printing the value to the logs - with appropriate explanation of the "big" values.
##########
File path: airflow/providers/google/leveldb/operators/leveldb.py
##########
@@ -92,7 +92,7 @@ def execute(self, context) -> Optional[str]:
keys=self.keys,
values=self.values,
)
- self.log.info("Done. Returned value was: %s", str(value))
+ self.log.debug("Done. Returned value was: %s", str(value))
leveldb_hook.close_conn()
value = value if value is None else value.decode()
Review comment:
Here I also think we need to do more: str(value) will actually perform string conversion no matter what is the log value (and it is not needed anyway because %s will do it anyway. So here both 'if' and removing str is 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] KennethanCeyer edited a comment on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer edited a comment on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-966898703
One quick question!
If the user uses `DecoratedOperator` as shown below through TaskFlow API,
there seems to be no way to block XCom return value logs.
```python
@task
def my_task(): # Guess it is `_PythonDecoratedOperator`
# ... process
return large_data
with DAG(...) as dag:
data = my_task() # It will make a huge log messages on Airflow UI
my_second_task(data)
```
so, I think additional work will be required in a separate PR for TaskFlow API users.
What do you think?
Below are the locations that appear to require further work.
https://github.com/apache/airflow/blob/a9772cf287111a63eac8c2deb1190f7054d7580f/airflow/decorators/python.py#L49-L59
--
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 #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #19378:
URL: https://github.com/apache/airflow/pull/19378
--
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] boring-cyborg[bot] commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-972044709
Awesome work, congrats on your first merged pull request!
--
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] KennethanCeyer commented on a change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r743517890
##########
File path: airflow/operators/python.py
##########
@@ -123,6 +123,9 @@ def my_python_callable(**kwargs):
:param templates_exts: a list of file extensions to resolve while
processing templated fields, for examples ``['.sql', '.hql']``
:type templates_exts: list[str]
+ :param show_return_value_in_logs: a bool value whether to show return_value
Review comment:
I just edited the docstring explanation, but
I will think about it a little more and improve it soon.
--
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 change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r747239756
##########
File path: airflow/operators/python.py
##########
@@ -145,6 +150,7 @@ def __init__(
op_kwargs: Optional[Dict] = None,
templates_dict: Optional[Dict] = None,
templates_exts: Optional[List[str]] = None,
+ show_return_value_in_logs: Optional[bool] = True,
Review comment:
If the default is `True`, this shouldn't need to be `Optional`? What does passing in `None` mean?
--
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] KennethanCeyer commented on a change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r747331574
##########
File path: airflow/operators/python.py
##########
@@ -171,7 +178,8 @@ def execute(self, context: Dict):
self.op_kwargs = determine_kwargs(self.python_callable, self.op_args, context)
return_value = self.execute_callable()
- self.log.info("Done. Returned value was: %s", return_value)
+ if self.show_return_value_in_logs:
+ self.log.info("Done. Returned value was: %s", return_value)
Review comment:
I will add a branch to display the phrase that it is turned off(`"Done. Returned value not shown"` which is mentioned by @potiuk) when the option is turned off.
--
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] KennethanCeyer commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958731328
--
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] KennethanCeyer commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958731328
--
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 pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-967047503
Yes we should do something for this but separately. Maybe something like (just a random idea)
```python
@task(..., log_return_value=False)
def my_task():
...
return large_data
```
--
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 #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-962666474
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] KennethanCeyer edited a comment on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer edited a comment on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-966898703
One quick question!
If the user uses `DecoratedOperator` as shown below through [TaskFlow API](https://airflow.apache.org/docs/apache-airflow/stable/tutorial_taskflow_api.html),
there seems to be no way to block XCom return value logs.
```python
@task
def my_task(): # Guess it is `_PythonDecoratedOperator`
# ... process
return large_data
with DAG(...) as dag:
data = my_task() # It will make a huge log messages on Airflow UI
my_second_task(data)
```
so, I think additional work will be required in a separate PR for TaskFlow API users.
What do you think?
Below are the locations that appear to require further work.
https://github.com/apache/airflow/blob/a9772cf287111a63eac8c2deb1190f7054d7580f/airflow/decorators/python.py#L49-L59
--
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] KennethanCeyer commented on a change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r747246524
##########
File path: airflow/operators/python.py
##########
@@ -145,6 +150,7 @@ def __init__(
op_kwargs: Optional[Dict] = None,
templates_dict: Optional[Dict] = None,
templates_exts: Optional[List[str]] = None,
+ show_return_value_in_logs: Optional[bool] = True,
Review comment:
Oops!
That's correct, I was mistaken.
I'll remove the Optional.
--
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 #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r741668839
##########
File path: airflow/operators/python.py
##########
@@ -171,7 +171,7 @@ def execute(self, context: Dict):
self.op_kwargs = determine_kwargs(self.python_callable, self.op_args, context)
return_value = self.execute_callable()
- self.log.info("Done. Returned value was: %s", return_value)
+ self.log.debug("Done. Returned value was: %s", return_value)
Review comment:
I do not think, changing the log level is good solution. There are many use cases where peopel might rely on the return value appear in logs.
I think better one might be to add optional parameter (default to True) `show_return_value_in_logs` that could be set to False tu suppress printing the value to the logs - with appropriate explanation of the "big" values.
--
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] boring-cyborg[bot] commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958673519
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] KennethanCeyer commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958737064
switch this PR to Draft due to the necessity to correct the related documents and write test specifications.
--
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] boring-cyborg[bot] commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958673519
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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 #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r747256097
##########
File path: airflow/operators/python.py
##########
@@ -171,7 +178,8 @@ def execute(self, context: Dict):
self.op_kwargs = determine_kwargs(self.python_callable, self.op_args, context)
return_value = self.execute_callable()
- self.log.info("Done. Returned value was: %s", return_value)
+ if self.show_return_value_in_logs:
+ self.log.info("Done. Returned value was: %s", return_value)
Review comment:
Good point - I think it's good to write explicily "Done. Returned value not shown".
--
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 #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r744303987
##########
File path: airflow/operators/python.py
##########
@@ -123,6 +123,9 @@ def my_python_callable(**kwargs):
:param templates_exts: a list of file extensions to resolve while
processing templated fields, for examples ``['.sql', '.hql']``
:type templates_exts: list[str]
+ :param show_return_value_in_logs: a bool value whether to show return_value
Review comment:
I think it's good enough. Hapy to merge it @KennethanCeyer if you make it ready for review (It's Draft) currently.
--
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 change in pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#discussion_r747248596
##########
File path: airflow/operators/python.py
##########
@@ -171,7 +178,8 @@ def execute(self, context: Dict):
self.op_kwargs = determine_kwargs(self.python_callable, self.op_args, context)
return_value = self.execute_callable()
- self.log.info("Done. Returned value was: %s", return_value)
+ if self.show_return_value_in_logs:
+ self.log.info("Done. Returned value was: %s", return_value)
Review comment:
I wonder we should still log something here (just donโt show the return value). No strong opinion here 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] KennethanCeyer commented on pull request #19378: Change logLevel as debug for XCOM returned value message
Posted by GitBox <gi...@apache.org>.
KennethanCeyer commented on pull request #19378:
URL: https://github.com/apache/airflow/pull/19378#issuecomment-958731328
@potiuk
Thanks for the quick comment.
The concern that this PR change will come as a side effect for many users seems valid.
I plan to do two things as follows.
1. Revert level one.
2. Rollback the logLevel `debug` to `info` of PythonOperator and Make a new optional parameter to control the display of the return_value log
(default is `True`, It means `display the return_value log`)
--
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