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/01/24 03:49:43 UTC
[GitHub] [airflow] josh-fell opened a new pull request #21054: Improve handling of string type and non-attribute `template_fields`
josh-fell opened a new pull request #21054:
URL: https://github.com/apache/airflow/pull/21054
Related: #19047
Closes: #12876
This PR intends to improve both the handling of `template_fields` values declared as strings (most typically seen as `template_fields = ("some_field")` as well as the error message for declaring `template_fields` that are not attributes of the operator. In the first case, a warning is logged and the `template_fields` is "automagically" converted to a list for the task execution. For the second case, the error message is updated from the default `AttributeError` to something more specific to what is happening when attempting to render any template fields.
---
**^ 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] josh-fell commented on pull request #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#issuecomment-1020380791
> > Pushed some fixes for static check errors...come on Josh. facepalm
>
> Hint:
>
> `pre-commit run --from-ref=HEAD^ --to-ref=HEAD`
>
> And a sneaky preview of new Breeze2 equivalent:
>
> `./Breeze2 static-checks --last-commit`
Yeah that's usually my go-to move but I completely spaced on it this time. That new command is lovely. I'll have to try it out.
--
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 #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#discussion_r790425035
##########
File path: airflow/models/baseoperator.py
##########
@@ -1085,10 +1095,16 @@ def _do_render_template_fields(
seen_oids: Set,
) -> None:
for attr_name in template_fields:
- content = getattr(parent, attr_name)
- if content:
- rendered_content = self.render_template(content, context, jinja_env, seen_oids)
- setattr(parent, attr_name, rendered_content)
+ if hasattr(parent, attr_name):
+ content = getattr(parent, attr_name)
+ if content:
+ rendered_content = self.render_template(content, context, jinja_env, seen_oids)
+ setattr(parent, attr_name, rendered_content)
+ else:
+ raise AttributeError(
+ f"{attr_name!r} is configured as a template field "
+ f"but {parent.task_type} does not have this attribute."
+ )
Review comment:
```suggestion
try:
content = getattr(parent, attr_name)
except AttributeError:
raise AttributeError(
f"{attr_name!r} is configured as a template field "
f"but {parent.task_type} does not have this attribute."
)
else:
if content:
rendered_content = self.render_template(content, context, jinja_env, seen_oids)
setattr(parent, attr_name, rendered_content)
```
EAFP over LBYL.
--
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 change in pull request #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#discussion_r790650103
##########
File path: airflow/models/baseoperator.py
##########
@@ -1085,10 +1095,16 @@ def _do_render_template_fields(
seen_oids: Set,
) -> None:
for attr_name in template_fields:
- content = getattr(parent, attr_name)
- if content:
- rendered_content = self.render_template(content, context, jinja_env, seen_oids)
- setattr(parent, attr_name, rendered_content)
+ if hasattr(parent, attr_name):
+ content = getattr(parent, attr_name)
+ if content:
+ rendered_content = self.render_template(content, context, jinja_env, seen_oids)
+ setattr(parent, attr_name, rendered_content)
+ else:
+ raise AttributeError(
+ f"{attr_name!r} is configured as a template field "
+ f"but {parent.task_type} does not have this attribute."
+ )
Review comment:
`except ... else` not 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 pull request #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#issuecomment-1020340584
cc: @Bowrna ^^
--
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] josh-fell commented on a change in pull request #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#discussion_r790783616
##########
File path: airflow/models/baseoperator.py
##########
@@ -742,6 +742,16 @@ def __init__(
]
)
+ if isinstance(self.template_fields, str):
+ warnings.warn(
+ f"The `template_fields` value for {self.task_type} is a string "
+ "but should be a Sequence type. Converting to a Sequence type for execution. "
+ f"Please update {self.task_type} accordingly.",
Review comment:
Excellent 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.
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 #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#discussion_r790425169
##########
File path: airflow/models/baseoperator.py
##########
@@ -1351,10 +1367,16 @@ def dry_run(self) -> None:
"""Performs dry run for the operator - just render template fields."""
self.log.info('Dry run')
for field in self.template_fields:
- content = getattr(self, field)
- if content and isinstance(content, str):
- self.log.info('Rendering template for %s', field)
- self.log.info(content)
+ if hasattr(self, field):
+ content = getattr(self, field)
+ if content and isinstance(content, str):
+ self.log.info('Rendering template for %s', field)
+ self.log.info(content)
+ else:
+ raise AttributeError(
+ f"{field!r} is configured as a template field "
+ f"but {self.task_type} does not have this attribute."
+ )
Review comment:
Same as 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] josh-fell commented on pull request #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#issuecomment-1020304788
Pushed some fixes for static check errors...come on Josh. 🤦
--
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 #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#discussion_r790424466
##########
File path: airflow/models/baseoperator.py
##########
@@ -742,6 +742,16 @@ def __init__(
]
)
+ if isinstance(self.template_fields, str):
+ warnings.warn(
+ f"The `template_fields` value for {self.task_type} is a string "
+ "but should be a Sequence type. Converting to a Sequence type for execution. "
+ f"Please update {self.task_type} accordingly.",
Review comment:
```suggestion
f"The `template_fields` value for {self.task_type} is a string "
"but should be a list of string. Wrapping it in a list for execution. "
f"Please update {self.task_type} accordingly.",
```
`str` is _technically_ a sequence, so _sequence of string_ is a better term. But I feel the term _sequence_ itself might not be the easiest to understand for less experienced developers, so let’s just recommend using a list instead, which is the most fool-proof syntax.
--
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 #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#discussion_r790424466
##########
File path: airflow/models/baseoperator.py
##########
@@ -742,6 +742,16 @@ def __init__(
]
)
+ if isinstance(self.template_fields, str):
+ warnings.warn(
+ f"The `template_fields` value for {self.task_type} is a string "
+ "but should be a Sequence type. Converting to a Sequence type for execution. "
+ f"Please update {self.task_type} accordingly.",
Review comment:
```suggestion
f"The `template_fields` value for {self.task_type} is a string "
"but should be a list of string. Wrapping it in a list for execution. "
f"Please update {self.task_type} accordingly.",
```
`str` is _technically_ a string, so _sequence of string_ is a better term. But I feel the term _sequence_ itself might not be the easiest to understand for less experienced developers, so let’s just recommend using a list instead, which is the most fool-proof syntax.
--
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 #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#issuecomment-1020338714
> Pushed some fixes for static check errors...come on Josh. facepalm
Hint:
`pre-commit run --from-ref=HEAD^ --to-ref=HEAD`
And a sneaky preview of new Breeze2 equivalent:
`./Breeze2 static-check --last-commit`
--
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 #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#issuecomment-1021928592
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] josh-fell commented on pull request #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#issuecomment-1020380791
> > Pushed some fixes for static check errors...come on Josh. facepalm
>
> Hint:
>
> `pre-commit run --from-ref=HEAD^ --to-ref=HEAD`
>
> And a sneaky preview of new Breeze2 equivalent:
>
> `./Breeze2 static-checks --last-commit`
Yeah that's usually my go-to move but I completely spaced on it this time. That new command is lovely. I'll have to try it out.
--
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] josh-fell commented on a change in pull request #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#discussion_r790784421
##########
File path: airflow/models/baseoperator.py
##########
@@ -1085,10 +1095,16 @@ def _do_render_template_fields(
seen_oids: Set,
) -> None:
for attr_name in template_fields:
- content = getattr(parent, attr_name)
- if content:
- rendered_content = self.render_template(content, context, jinja_env, seen_oids)
- setattr(parent, attr_name, rendered_content)
+ if hasattr(parent, attr_name):
+ content = getattr(parent, attr_name)
+ if content:
+ rendered_content = self.render_template(content, context, jinja_env, seen_oids)
+ setattr(parent, attr_name, rendered_content)
+ else:
+ raise AttributeError(
+ f"{attr_name!r} is configured as a template field "
+ f"but {parent.task_type} does not have this attribute."
+ )
Review comment:
Ah yes. More succinct as well.
--
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 edited a comment on pull request #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21054:
URL: https://github.com/apache/airflow/pull/21054#issuecomment-1020338714
> Pushed some fixes for static check errors...come on Josh. facepalm
Hint:
`pre-commit run --from-ref=HEAD^ --to-ref=HEAD`
And a sneaky preview of new Breeze2 equivalent:
`./Breeze2 static-checks --last-commit`
--
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 #21054: Improve handling of string type and non-attribute `template_fields`
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #21054:
URL: https://github.com/apache/airflow/pull/21054
--
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