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/09/30 07:20:59 UTC
[GitHub] [airflow] eladkal opened a new pull request #18627: Add default weight rule configuration option
eladkal opened a new pull request #18627:
URL: https://github.com/apache/airflow/pull/18627
closes: https://github.com/apache/airflow/issues/12807
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a change in pull request #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719175162
##########
File path: airflow/models/baseoperator.py
##########
@@ -497,7 +497,7 @@ def __init__(
params: Optional[Dict] = None,
default_args: Optional[Dict] = None,
priority_weight: int = 1,
- weight_rule: str = WeightRule.DOWNSTREAM,
+ weight_rule: str = conf.get('core', 'default_task_weight_rule'),
Review comment:
added
--
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] eladkal commented on a change in pull request #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719337736
##########
File path: airflow/configuration.py
##########
@@ -229,6 +234,14 @@ def validate(self):
self.is_validated = True
+ def _validate_enums(self):
+ """Validate that enum type config has an accepted value"""
+ for setting, enum_class in self.enums.items():
+ if self.has_option('core', setting):
+ value = self.get("core", setting)
+ if not enum_class.is_valid(value):
Review comment:
yeah that should be done after https://github.com/apache/airflow/pull/5302
--
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 #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719304267
##########
File path: airflow/config_templates/default_airflow.cfg
##########
@@ -207,6 +207,9 @@ dag_discovery_safe_mode = True
# The number of retries each task is going to have by default. Can be overridden at dag or task level.
default_task_retries = 0
+# The weighting method used for the effective total priority weight of the task
+default_task_weight_rule = downstream
Review comment:
Oh. Though it turns out WeightRule isn't actually an Enum. Go figure.
--
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 #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719305429
##########
File path: airflow/configuration.py
##########
@@ -229,6 +234,14 @@ def validate(self):
self.is_validated = True
+ def _validate_enums(self):
+ """Validate that enum type config has an accepted value"""
+ for setting, enum_class in self.enums.items():
+ if self.has_option('core', setting):
+ value = self.get("core", setting)
+ if not enum_class.is_valid(value):
Review comment:
For an actaul enum (i.e. one inheriting from `enum.Enum`) the test for this is
```
try:
enum_class(value)
except ValueError:
raise AirflowConfigException(f"{value} is not an accepted config for {setting}") from None
```
--
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 #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719303760
##########
File path: airflow/config_templates/default_airflow.cfg
##########
@@ -207,6 +207,9 @@ dag_discovery_safe_mode = True
# The number of retries each task is going to have by default. Can be overridden at dag or task level.
default_task_retries = 0
+# The weighting method used for the effective total priority weight of the task
+default_task_weight_rule = downstream
Review comment:
Yup, 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719167263
##########
File path: airflow/models/baseoperator.py
##########
@@ -497,7 +497,7 @@ def __init__(
params: Optional[Dict] = None,
default_args: Optional[Dict] = None,
priority_weight: int = 1,
- weight_rule: str = WeightRule.DOWNSTREAM,
+ weight_rule: str = conf.get('core', 'default_task_weight_rule'),
Review comment:
```suggestion
weight_rule: str = conf.get('core', 'default_task_weight_rule', fallback=WeightRule.DOWNSTREAM),
```
Should we have a fallback too?
--
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] eladkal commented on a change in pull request #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719301385
##########
File path: airflow/config_templates/default_airflow.cfg
##########
@@ -207,6 +207,9 @@ dag_discovery_safe_mode = True
# The number of retries each task is going to have by default. Can be overridden at dag or task level.
default_task_retries = 0
+# The weighting method used for the effective total priority weight of the task
+default_task_weight_rule = downstream
Review comment:
is https://github.com/apache/airflow/pull/18627/commits/da521bd820ead09cfd6fa1d1b8f1c73f16a66c78 what you had in mind?
--
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] eladkal commented on a change in pull request #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719334625
##########
File path: airflow/config_templates/default_airflow.cfg
##########
@@ -207,6 +207,9 @@ dag_discovery_safe_mode = True
# The number of retries each task is going to have by default. Can be overridden at dag or task level.
default_task_retries = 0
+# The weighting method used for the effective total priority weight of the task
+default_task_weight_rule = downstream
Review comment:
It was supposed to be converted to Enum in https://github.com/apache/airflow/pull/5302
--
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 #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#issuecomment-930992117
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719210113
##########
File path: airflow/config_templates/default_airflow.cfg
##########
@@ -207,6 +207,9 @@ dag_discovery_safe_mode = True
# The number of retries each task is going to have by default. Can be overridden at dag or task level.
default_task_retries = 0
+# The weighting method used for the effective total priority weight of the task
+default_task_weight_rule = downstream
Review comment:
We should probably validate that this option is valid (i.e. it's in the enum) in airflow.configuration.Configration.validate()
--
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] eladkal merged pull request #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
eladkal merged pull request #18627:
URL: https://github.com/apache/airflow/pull/18627
--
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 #18627: Add default weight rule configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18627:
URL: https://github.com/apache/airflow/pull/18627#discussion_r719302604
##########
File path: airflow/configuration.py
##########
@@ -193,6 +194,8 @@ class AirflowConfigParser(ConfigParser):
},
}
+ enums = {"default_task_weight_rule": WeightRule}
Review comment:
```suggestion
enums = {("core", "default_task_weight_rule"): WeightRule}
```
##########
File path: airflow/configuration.py
##########
@@ -229,6 +234,14 @@ def validate(self):
self.is_validated = True
+ def _validate_enums(self):
+ """Validate that enum type config has an accepted value"""
+ for setting, enum_class in self.enums.items():
+ if self.has_option('core', setting):
+ value = self.get("core", setting)
Review comment:
```suggestion
for (section, setting), enum_class in self.enums.items():
if self.has_option(section, setting):
value = self.get(section, setting)
```
##########
File path: airflow/configuration.py
##########
@@ -229,6 +234,14 @@ def validate(self):
self.is_validated = True
+ def _validate_enums(self):
+ """Validate that enum type config has an accepted value"""
+ for setting, enum_class in self.enums.items():
+ if self.has_option('core', setting):
+ value = self.get("core", setting)
+ if not enum_class.is_valid(value):
+ raise AirflowConfigException(f"{value} is not an accepted config for {setting}")
Review comment:
```suggestion
raise AirflowConfigException(f"{value} is not an accepted config for [{section}] {setting}")
```
--
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