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/12/09 16:49:09 UTC
[GitHub] [airflow] dstandish opened a new pull request #20174: Add deprecation warnining for non-json-serializable params
dstandish opened a new pull request #20174:
URL: https://github.com/apache/airflow/pull/20174
Following vote by lazy consensus (https://lists.apache.org/thread/whqbbo3gh84s7ggn7968mqlk4d41x7zl), we deprecate non-json-serializable params with removal planned for Airflow 3.0.
--
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 #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#discussion_r769243345
##########
File path: airflow/models/param.py
##########
@@ -49,10 +51,28 @@ def __init__(self, default: Any = NOTSET, description: Optional[str] = None, **k
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
+ if self.has_value:
+ self._validate(self.value, self.schema)
+
+ @staticmethod
+ def _validate(value, schema):
+ """
+ 1. Check that value is json-serializable; if not, warn. In future release we will require
+ the value to be json-serializable.
+ 2. Validate ``value`` against ``schema``
+ """
+ try:
+ json.dumps(value)
+ except Exception:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in "
+ " a future release",
+ DeprecationWarning,
+ )
Review comment:
```suggestion
warnings.warn(
"The use of non-json-serializable params is deprecated and will be removed in "
"a future release",
DeprecationWarning,
)
```
Also I think we should add `stacklevel` to make the warning show where the user defines the params.
--
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] jedcunningham commented on a change in pull request #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#discussion_r765988392
##########
File path: airflow/models/param.py
##########
@@ -57,10 +59,28 @@ def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: Optional[str
# If we have a value, validate it once. May raise ValueError.
if self.has_value:
- try:
- jsonschema.validate(self.value, self.schema, format_checker=FormatChecker())
- except ValidationError as err:
- raise ValueError(err)
+ self._validate(self.value, self.schema)
+
+ @staticmethod
+ def _validate(value, schema):
+ """
+ 1. Check that value is json-serializable; if not, warn. In future release we will require
+ the value to be json-serializable.
+ 2. Validate ``value`` against ``schema``
+ """
+ try:
+ json.dumps(value)
+ except Exception:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in "
+ " a future release",
+ DeprecationWarning,
+ stacklevel=2,
Review comment:
Is this deep enough?
--
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] dstandish commented on a change in pull request #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#discussion_r767889965
##########
File path: airflow/models/param.py
##########
@@ -57,10 +59,28 @@ def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: Optional[str
# If we have a value, validate it once. May raise ValueError.
if self.has_value:
- try:
- jsonschema.validate(self.value, self.schema, format_checker=FormatChecker())
- except ValidationError as err:
- raise ValueError(err)
+ self._validate(self.value, self.schema)
+
+ @staticmethod
+ def _validate(value, schema):
+ """
+ 1. Check that value is json-serializable; if not, warn. In future release we will require
+ the value to be json-serializable.
+ 2. Validate ``value`` against ``schema``
+ """
+ try:
+ json.dumps(value)
+ except Exception:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in "
+ " a future release",
+ DeprecationWarning,
+ stacklevel=2,
Review comment:
nudge?
--
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] dstandish commented on a change in pull request #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#discussion_r769285903
##########
File path: airflow/models/param.py
##########
@@ -49,10 +51,28 @@ def __init__(self, default: Any = NOTSET, description: Optional[str] = None, **k
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
+ if self.has_value:
+ self._validate(self.value, self.schema)
+
+ @staticmethod
+ def _validate(value, schema):
+ """
+ 1. Check that value is json-serializable; if not, warn. In future release we will require
+ the value to be json-serializable.
+ 2. Validate ``value`` against ``schema``
+ """
+ try:
+ json.dumps(value)
+ except Exception:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in "
+ " a future release",
+ DeprecationWarning,
+ )
Review comment:
I experimented with different values of stacklevel and it seems it's not possible to make it reach the place of definition. It's too nested it seems.
--
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] dstandish commented on pull request #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#issuecomment-992647325
> lgtm.. plz add a supporting testcase as well.
done [here](https://github.com/apache/airflow/pull/20174/files#diff-14774aab4b848314e7a3f6ea3e98f85d83dd1220076265a50db78850e38d22b1R271) 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] jedcunningham commented on a change in pull request #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#discussion_r767933969
##########
File path: airflow/models/param.py
##########
@@ -57,10 +59,28 @@ def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: Optional[str
# If we have a value, validate it once. May raise ValueError.
if self.has_value:
- try:
- jsonschema.validate(self.value, self.schema, format_checker=FormatChecker())
- except ValidationError as err:
- raise ValueError(err)
+ self._validate(self.value, self.schema)
+
+ @staticmethod
+ def _validate(value, schema):
+ """
+ 1. Check that value is json-serializable; if not, warn. In future release we will require
+ the value to be json-serializable.
+ 2. Validate ``value`` against ``schema``
+ """
+ try:
+ json.dumps(value)
+ except Exception:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in "
+ " a future release",
+ DeprecationWarning,
+ stacklevel=2,
Review comment:
Yeah, stacklevel. We want it to point to someplace meaningful for the user, otherwise we shouldn't skip any.
(sorry, not sure how I missed this last week)
--
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 #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#issuecomment-990045560
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] dstandish commented on a change in pull request #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#discussion_r767995608
##########
File path: airflow/models/param.py
##########
@@ -57,10 +59,28 @@ def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: Optional[str
# If we have a value, validate it once. May raise ValueError.
if self.has_value:
- try:
- jsonschema.validate(self.value, self.schema, format_checker=FormatChecker())
- except ValidationError as err:
- raise ValueError(err)
+ self._validate(self.value, self.schema)
+
+ @staticmethod
+ def _validate(value, schema):
+ """
+ 1. Check that value is json-serializable; if not, warn. In future release we will require
+ the value to be json-serializable.
+ 2. Validate ``value`` against ``schema``
+ """
+ try:
+ json.dumps(value)
+ except Exception:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in "
+ " a future release",
+ DeprecationWarning,
+ stacklevel=2,
Review comment:
chopped `stacklevel` param
--
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] dstandish merged pull request #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
dstandish merged pull request #20174:
URL: https://github.com/apache/airflow/pull/20174
--
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] dstandish commented on a change in pull request #20174: Add deprecation warnining for non-json-serializable params
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20174:
URL: https://github.com/apache/airflow/pull/20174#discussion_r766001589
##########
File path: airflow/models/param.py
##########
@@ -57,10 +59,28 @@ def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: Optional[str
# If we have a value, validate it once. May raise ValueError.
if self.has_value:
- try:
- jsonschema.validate(self.value, self.schema, format_checker=FormatChecker())
- except ValidationError as err:
- raise ValueError(err)
+ self._validate(self.value, self.schema)
+
+ @staticmethod
+ def _validate(value, schema):
+ """
+ 1. Check that value is json-serializable; if not, warn. In future release we will require
+ the value to be json-serializable.
+ 2. Validate ``value`` against ``schema``
+ """
+ try:
+ json.dumps(value)
+ except Exception:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in "
+ " a future release",
+ DeprecationWarning,
+ stacklevel=2,
Review comment:
Do you mean the stackllevel param? I just used what we use elsewhere...
--
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