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/10/27 23:59:17 UTC
[GitHub] [airflow] dstandish opened a new pull request #19267: Fix serialization of Params with set data type
dstandish opened a new pull request #19267:
URL: https://github.com/apache/airflow/pull/19267
OK this may be a solution for https://github.com/apache/airflow/issues/19096
Though I'll appreciate comment on the approach. Just wanted to get something working initially 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] dstandish commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743320052
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -1433,29 +1461,32 @@ def test_serialized_objects_are_sorted(self, object_to_serialized, expected_outp
assert serialized_obj == expected_output
def test_params_upgrade(self):
+ """when pre-2.2.0 param (i.e. primitive) is deserialized we convert to Param"""
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
- "fileloc": __file__,
+ "fileloc": '__file__',
Review comment:
this value actually does not have any impact on the test. it could be `/hello/world.py`
but when you put `__file__` you can't run the bit of code interactively.
```python
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
"fileloc": __file__,
"tasks": [],
"timezone": "UTC",
"params": {"none": None, "str": "str", "dict": {"a": "b"}},
},
}
dag = SerializedDAG.from_dict(serialized)
Traceback (most recent call last):
File "/Users/dstandish/.virtualenvs/local/lib/python3.8/site-packages/IPython/core/interactiveshell.py", line 3444, in run_code
exec(code_obj, self.user_global_ns, self.user_ns)
File "<ipython-input-2-4ee74d1eaf6b>", line 5, in <module>
"fileloc": __file__,
NameError: name '__file__' is not defined
```
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process not only converts to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately serializes to json. but the test previously did not convert to json it only to dict and back. and this is actually one of the reasons we did not catch the `set` issue, because itt worked for to_dict but not for to_json.
so here we first go all the way to json before deserilaizing. in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process must convert not only to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately must serialize to json.
the test previously did not convert to json; only to dict and back. and this is actually one of the reasons we did not catch the `set` issue, because it worked for to_dict but not for to_json.
so here we first go all the way to json before deserilaizing.
in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
ok sure.
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
done
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process must convert not only to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately must serialize to json.
the test previously did not convert to json; only to dict and back. so without making this change, it would not catch the `set` issue.
so here we first go all the way to json before deserilaizing.
in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #19267:
URL: https://github.com/apache/airflow/pull/19267
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738954322
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
What is causing the KeyError? It feels wrong to just "give up" on deserializing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741499035
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
I remember why we hadn't included this file yet in above pre-commit, we formatted in a way that was a bit easy to read.
Example:
vs
However no strong preference
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
Why is this separate than the above pre-commit and why `2` indent instead of `4`
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
In any case, let's separate the formatting and pre-commit change to a different PR than the bugfix please
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
Thinking about this more: @msumit do you think we will be able to validate `set` or non-JSON structures?
--
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] msumit commented on pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-958634168
lgtm overall. Also nice work in handling the backward compatibility 👍
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-953981128
there's a bit of awkwardness with the `Param` object as relates to serialization because, while for other objects we can reuse __dict__ as kwargs, for params, `default` is a kwarg but not an attribute. it is tempting to add `default` as an attribute and let `value` be None, or perhaps a property, but not sure of downstream consequences.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738684758
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,16 +407,26 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ param_dict = param.dump()
+ param_dict['default'] = cls._serialize(param_dict.pop('value'))
+ return param_dict
+
+ @classmethod
+ def _deserialize_param(cls, param_dict: Dict):
+ param_class = import_string(param_dict['__class'])
+ param_dict['default'] = cls._deserialize(param_dict['default'])
Review comment:
Should we be doing `_deserialize` on everything in `param_dict`?
--
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] msumit commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741599621
##########
File path: airflow/models/param.py
##########
@@ -49,6 +49,7 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
Review comment:
In the original PR, I initially used `__type` only, but Ash felt that it should be called `__class`, cause it would be representing a Param class & not a native type, which made sense to me.
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
Do we really need to serialize/deserialize a parma's description as well? It ought to be a simple string only.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r739259049
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
and another reason to be skeptical of this "fix" -- the big feature here is that params can have validation logic built-in. but the validation approach is [json schema validation](https://json-schema.org/draft/2020-12/json-schema-validation.html), i.e. it explicitly assumes you are validating json! so why would we allow params to not be json (or json-serializable).
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-954349180
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738962350
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
that's not what's going on here
the KeyError is essentially 2.2.0 serialiazation approach, which for params, doesn't run their attrs through our serialization logic, but assumes they are json serializable (which is the problem, because in the case of sets they are not)
but so when you try to _deserialize something that was not _serialize(d), you get a KeyError and in this case we can just take the args as they are -- they weren't "serialized" anyway, so we don't need to "deserialize" them
_however_, i am working on a better approach, cleaner, and i'll push it tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #19267:
URL: https://github.com/apache/airflow/pull/19267
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743321614
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process not only converts to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately serializes to json. but the test previously did not convert to json it only to dict and back. and this is actually one of the reasons we did not catch the `set` issue, because itt worked for to_dict but not for to_json.
so here we first go all the way to json before deserilaizing. in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743321614
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process must convert not only to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately must serialize to json.
the test previously did not convert to json; only to dict and back. and this is actually one of the reasons we did not catch the `set` issue, because it worked for to_dict but not for to_json.
so here we first go all the way to json before deserilaizing.
in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
--
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] msumit commented on pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-958634168
lgtm overall. Also nice work in handling the backward compatibility 👍
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-960502099
fyi i updated Param init to check whether the `default` is json serializable and a deprecation warning if it is not.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741650042
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
ok just sat down to not `_serialize` the `description` attr. and on second thought, i think it's better to run it through `_serialize` after all. like, that's precisely what `_serialize` is there for. there's a reason that `_serialize` has handling for primitive types, and so we should use it; to do otherwise is essentially to duplicate serialization logic. not running through `_serialize` would not prevent a user from using other json-serializable types (only would prevent from using non-json-ser types like `set`) -- if we really want to clamp down on types, probably the right place do do that is in `Parame.__init__`.
so, i think i'll leave this as is, but let me know if you think otherwise.
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] dstandish edited a comment on pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-953981128
there's a bit of awkwardness with the `Param` object as relates to serialization because, while for other objects we can reuse __dict__ as kwargs, for params, `default` is a kwarg but not an attribute. it is tempting to add `default` as an attribute perhaps make `value` a property or just remove it, but not sure of downstream consequences.
--
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] msumit commented on pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-958634168
lgtm overall. Also nice work in handling the backward compatibility 👍
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738725964
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,16 +407,26 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ param_dict = param.dump()
+ param_dict['default'] = cls._serialize(param_dict.pop('value'))
+ return param_dict
+
+ @classmethod
+ def _deserialize_param(cls, param_dict: Dict):
+ param_class = import_string(param_dict['__class'])
+ param_dict['default'] = cls._deserialize(param_dict['default'])
Review comment:
yeah probably so...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741499035
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
I remember why we hadn't included this file yet in above pre-commit, we formatted in a way that was a bit easy to read.
Example:
vs
However no strong preference
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
Why is this separate than the above pre-commit and why `2` indent instead of `4`
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
In any case, let's separate the formatting and pre-commit change to a different PR than the bugfix please
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
Thinking about this more: @msumit do you think we will be able to validate `set` or non-JSON structures?
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
Thinking about this more: @msumit do you think we will be able to validate `set` or non-JSON structures using the JSON Schema validation?
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
I remember why we hadn't included this file yet in above pre-commit, we formatted in a way that was a bit easy to read.
Example:
vs
However no strong preference
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
Why is this separate than the above pre-commit and why `2` indent instead of `4`
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
In any case, let's separate the formatting and pre-commit change to a different PR than the bugfix please
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
Thinking about this more: @msumit do you think we will be able to validate `set` or non-JSON structures?
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
Thinking about this more: @msumit do you think we will be able to validate `set` or non-JSON structures using the JSON Schema validation?
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-960373935
> @dstandish Can you take a look at the failing tests please
yup will do
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741501000
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
Thinking about this more: @msumit do you think we will be able to validate `set` or non-JSON structures using the JSON Schema validation?
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741608516
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
So I think it ends up being the same because string is primitive type
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
yeah just double checked and when it is primitive type, `_serialize` doesn't do anything
though, maybe we should not serialize it so as not to accidentally support something users should not be doing anyway. so on second thought, probably should change this to put the raw value (in which case, json-serialization will fail)
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
ok just sat down to not `_serialize` the `description` attr. and on second thought, i think it's better to run it through `_serialize` after all. like, that's precisely what `_serialize` is there for. there's a reason that `_serialize` has handling for primitive types, and so we should use it; to do otherwise is essentially to duplicate serialization logic. not running through `_serialize` would not prevent a user from using other json-serializable types (only would prevent from using non-json-ser types like `set`) -- if we really want to clamp down on types, probably the right place do do that is in `Parame.__init__`.
so, i think i'll leave this as is, but let me know if you think otherwise.
thanks
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
ok just sat down to not `_serialize` the `description` attr. and on second thought, i think it's better to run it through `_serialize` after all. like, that's precisely what `_serialize` is there for. there's a reason that `_serialize` has handling for primitive types, and so we should use it; to do otherwise is essentially to duplicate serialization logic. also, not running through `_serialize` would not achieve the goal of preventing a user from using non-string values (it only would prevent from using non-json-serializable types such as `set`) -- if we really want to clamp down on types, probably the right place do do that is in `Param.__init__`.
so, i think i'll leave this as is, but let me know if you think otherwise.
thanks
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
yeah i'll split it out.
but yeah it was previously not processed in pre-commit. but it should be to ensure consistent formatting, even if that means slightly less pretty.
and yeah i just chose indent of 2 because that's what the file already had.
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
can you clarify what you mean @kaxil ? the serializer converts them to json...
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
ok updated
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
So I think it ends up being the same because string is primitive type
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
yeah just double checked and when it is primitive type, `_serialize` doesn't do anything
though, maybe we should not serialize it so as not to accidentally support something users should not be doing anyway. so on second thought, probably should change this to put the raw value (in which case, json-serialization will fail)
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
ok just sat down to not `_serialize` the `description` attr. and on second thought, i think it's better to run it through `_serialize` after all. like, that's precisely what `_serialize` is there for. there's a reason that `_serialize` has handling for primitive types, and so we should use it; to do otherwise is essentially to duplicate serialization logic. not running through `_serialize` would not prevent a user from using other json-serializable types (only would prevent from using non-json-ser types like `set`) -- if we really want to clamp down on types, probably the right place do do that is in `Parame.__init__`.
so, i think i'll leave this as is, but let me know if you think otherwise.
thanks
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
ok just sat down to not `_serialize` the `description` attr. and on second thought, i think it's better to run it through `_serialize` after all. like, that's precisely what `_serialize` is there for. there's a reason that `_serialize` has handling for primitive types, and so we should use it; to do otherwise is essentially to duplicate serialization logic. also, not running through `_serialize` would not achieve the goal of preventing a user from using non-string values (it only would prevent from using non-json-serializable types such as `set`) -- if we really want to clamp down on types, probably the right place do do that is in `Param.__init__`.
so, i think i'll leave this as is, but let me know if you think otherwise.
thanks
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
yeah i'll split it out.
but yeah it was previously not processed in pre-commit. but it should be to ensure consistent formatting, even if that means slightly less pretty.
and yeah i just chose indent of 2 because that's what the file already had.
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
can you clarify what you mean @kaxil ? the serializer converts them to json...
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
ok updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #19267:
URL: https://github.com/apache/airflow/pull/19267
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738524870
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,16 +407,26 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ d = param.dump()
+ d['value'] = cls._serialize(d['value'])
+ return d
+
+ @classmethod
+ def _deserialize_param(cls, param: Dict):
+ param_class = import_string(param['__class'])
+ param['value'] = cls._deserialize(param['value'])
+ return param_class(default=param['value'], description=param['description'])
Review comment:
it's a good point, have to look into that
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738579638
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,16 +407,26 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ d = param.dump()
+ d['value'] = cls._serialize(d['value'])
+ return d
+
+ @classmethod
+ def _deserialize_param(cls, param: Dict):
+ param_class = import_string(param['__class'])
+ param['value'] = cls._deserialize(param['value'])
+ return param_class(default=param['value'], description=param['description'])
Review comment:
ok in latest commit i think i fixed my fix, so that it should be no change to serialization structure. initially i had used `value` but now i restore `default` as the key. theres just ambiguity because `default` is the kwarg name, and `value` is the attr.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738882776
##########
File path: airflow/models/param.py
##########
@@ -49,6 +49,7 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
Review comment:
I think that would be a schema change.
Note that type is used for deciding how to serialize, but in this case, class is one layer below. It's probably redundant and could possibly be combined but this is the existing structure
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738963140
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
separately though, do we really need to support non-json-serializable params? this will cause problems, for example when you run trigger dag in the UI -- probably in that case they _must_ be valid json. so what's the point of supporting non-json in the default? i think it would be best if we simply require that params be valid json.
--
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] msumit commented on pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-958634168
lgtm overall. Also nice work in handling the backward compatibility 👍
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741608516
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
So I think it ends up being the same because string is primitive type
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r742312470
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
can you clarify what you mean @kaxil ? the serializer converts them to json...
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741608516
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
So I think it ends up being the same because string is primitive type
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
yeah just double checked and when it is primitive type, `_serialize` doesn't do anything
though, maybe we should not serialize it so as not to accidentally support something users should not be doing anyway. so on second thought, probably should change this to put the raw value (in which case, json-serialization will fail)
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
ok just sat down to not `_serialize` the `description` attr. and on second thought, i think it's better to run it through `_serialize` after all. like, that's precisely what `_serialize` is there for. there's a reason that `_serialize` has handling for primitive types, and so we should use it; to do otherwise is essentially to duplicate serialization logic. not running through `_serialize` would not prevent a user from using other json-serializable types (only would prevent from using non-json-ser types like `set`) -- if we really want to clamp down on types, probably the right place do do that is in `Parame.__init__`.
so, i think i'll leave this as is, but let me know if you think otherwise.
thanks
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
ok just sat down to not `_serialize` the `description` attr. and on second thought, i think it's better to run it through `_serialize` after all. like, that's precisely what `_serialize` is there for. there's a reason that `_serialize` has handling for primitive types, and so we should use it; to do otherwise is essentially to duplicate serialization logic. also, not running through `_serialize` would not achieve the goal of preventing a user from using non-string values (it only would prevent from using non-json-serializable types such as `set`) -- if we really want to clamp down on types, probably the right place do do that is in `Param.__init__`.
so, i think i'll leave this as is, but let me know if you think otherwise.
thanks
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
yeah i'll split it out.
but yeah it was previously not processed in pre-commit. but it should be to ensure consistent formatting, even if that means slightly less pretty.
and yeah i just chose indent of 2 because that's what the file already had.
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
can you clarify what you mean @kaxil ? the serializer converts them to json...
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
ok updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743270792
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
Any reason we had to change this?
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
Let's separate deprecation from this PR -- so we can think more about it.
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -1433,29 +1461,32 @@ def test_serialized_objects_are_sorted(self, object_to_serialized, expected_outp
assert serialized_obj == expected_output
def test_params_upgrade(self):
+ """when pre-2.2.0 param (i.e. primitive) is deserialized we convert to Param"""
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
- "fileloc": __file__,
+ "fileloc": '__file__',
Review comment:
Why this change?
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
Any reason we had to change this?
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
Let's separate deprecation from this PR -- so we can think more about it.
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -1433,29 +1461,32 @@ def test_serialized_objects_are_sorted(self, object_to_serialized, expected_outp
assert serialized_obj == expected_output
def test_params_upgrade(self):
+ """when pre-2.2.0 param (i.e. primitive) is deserialized we convert to Param"""
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
- "fileloc": __file__,
+ "fileloc": '__file__',
Review comment:
Why this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743270792
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
Any reason we had to change this?
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
Let's separate deprecation from this PR -- so we can think more about it.
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -1433,29 +1461,32 @@ def test_serialized_objects_are_sorted(self, object_to_serialized, expected_outp
assert serialized_obj == expected_output
def test_params_upgrade(self):
+ """when pre-2.2.0 param (i.e. primitive) is deserialized we convert to Param"""
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
- "fileloc": __file__,
+ "fileloc": '__file__',
Review comment:
Why this change?
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738884166
##########
File path: airflow/models/param.py
##########
@@ -49,6 +49,7 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
Review comment:
Oh! So the inconsistency is already there (in 2.2). In that case I guess it's better to stick to it.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738751670
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,16 +407,26 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ param_dict = param.dump()
+ param_dict['default'] = cls._serialize(param_dict.pop('value'))
+ return param_dict
+
+ @classmethod
+ def _deserialize_param(cls, param_dict: Dict):
+ param_class = import_string(param_dict['__class'])
+ param_dict['default'] = cls._deserialize(param_dict['default'])
Review comment:
ok updated
--
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] msumit commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741601608
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
Do we really need to serialize/deserialize a parma's description as well? It ought to be a simple string only.
--
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] msumit commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741599621
##########
File path: airflow/models/param.py
##########
@@ -49,6 +49,7 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
Review comment:
In the original PR, I initially used `__type` only, but Ash felt that it should be called `__class`, cause it would be representing a Param class & not a native type, which made sense to me.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743322065
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
ok sure.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743320052
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -1433,29 +1461,32 @@ def test_serialized_objects_are_sorted(self, object_to_serialized, expected_outp
assert serialized_obj == expected_output
def test_params_upgrade(self):
+ """when pre-2.2.0 param (i.e. primitive) is deserialized we convert to Param"""
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
- "fileloc": __file__,
+ "fileloc": '__file__',
Review comment:
this value actually does not have any impact on the test. it could be `/hello/world.py`
but when you put `__file__` you can't run the bit of code interactively.
```python
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
"fileloc": __file__,
"tasks": [],
"timezone": "UTC",
"params": {"none": None, "str": "str", "dict": {"a": "b"}},
},
}
dag = SerializedDAG.from_dict(serialized)
Traceback (most recent call last):
File "/Users/dstandish/.virtualenvs/local/lib/python3.8/site-packages/IPython/core/interactiveshell.py", line 3444, in run_code
exec(code_obj, self.user_global_ns, self.user_ns)
File "<ipython-input-2-4ee74d1eaf6b>", line 5, in <module>
"fileloc": __file__,
NameError: name '__file__' is not defined
```
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process not only converts to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately serializes to json. but the test previously did not convert to json it only to dict and back. and this is actually one of the reasons we did not catch the `set` issue, because itt worked for to_dict but not for to_json.
so here we first go all the way to json before deserilaizing. in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process must convert not only to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately must serialize to json.
the test previously did not convert to json; only to dict and back. and this is actually one of the reasons we did not catch the `set` issue, because it worked for to_dict but not for to_json.
so here we first go all the way to json before deserilaizing.
in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
ok sure.
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
done
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process must convert not only to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately must serialize to json.
the test previously did not convert to json; only to dict and back. so without making this change, it would not catch the `set` issue.
so here we first go all the way to json before deserilaizing.
in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743376540
##########
File path: airflow/models/param.py
##########
@@ -49,18 +50,37 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
def __init__(self, default: Any = __NO_VALUE_SENTINEL, description: str = None, **kwargs):
+
self.value = default
self.description = description
self.schema = kwargs.pop('schema') if 'schema' in kwargs else kwargs
- # 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 TypeError:
+ warnings.warn(
+ "The use of non-json-serializable params is deprecated and will be removed in a"
+ " future release",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
done
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738050986
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,16 +407,26 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ d = param.dump()
+ d['value'] = cls._serialize(d['value'])
+ return d
+
+ @classmethod
+ def _deserialize_param(cls, param: Dict):
+ param_class = import_string(param['__class'])
+ param['value'] = cls._deserialize(param['value'])
+ return param_class(default=param['value'], description=param['description'])
Review comment:
How does this work with DAGs already serialised in the database? Do we need to implement some detection to ensure those entries can be deserialised correctly?
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743321614
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -733,7 +735,10 @@ def test_dag_params_roundtrip(self, val, expected_val):
dag = DAG(dag_id='simple_dag', params=val)
BaseOperator(task_id='simple_task', dag=dag, start_date=datetime(2019, 8, 1))
- serialized_dag = SerializedDAG.to_dict(dag)
+ serialized_dag_json = SerializedDAG.to_json(dag)
+
+ serialized_dag = json.loads(serialized_dag_json)
Review comment:
by calling `to_json` we make this test stricter. the full serialization process must convert not only to a dictionary (which is python, thus can handle arbitrary data types) but also ultimately must serialize to json.
the test previously did not convert to json; only to dict and back. so without making this change, it would not catch the `set` issue.
so here we first go all the way to json before deserilaizing.
in other words, previously the round trip wasn't really a full round trip and that's what this change fixes.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741628749
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
yeah just double checked and when it is primitive type, `_serialize` doesn't do anything
though, maybe we should not serialize it so as not to accidentally support something users should not be doing anyway. so on second thought, probably should change this to put the raw value (in which case, json-serialization will fail)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r742463759
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
The following code runs jsonschema validation if one is supplied.
https://github.com/apache/airflow/blob/02b7e2c092902bf42791a5ef1a70bc71226f1e32/airflow/models/param.py#L47-L51
Example test:
https://github.com/apache/airflow/blob/02b7e2c092902bf42791a5ef1a70bc71226f1e32/tests/models/test_param.py#L58-L66
This won't be possible with "set" is what I mean:
i.e. the following works:
```python
def test_set_param(self):
p = Param({'a', 'b'})
assert p.resolve() == {'a', 'b'}
```
but there is no way of adding validation like the set only contains <10 elements, example:
```python
def test_set_param(self):
p = Param({'a', 'b'}, type='set', maxItems=1)
assert p.resolve() == {'a', 'b'}
```
**It is not a big deal but worth pointing that out in the docs**
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
I remember why we hadn't included this file yet in above pre-commit, we formatted in a way that was a bit easy to read.
Example:
vs
However no strong preference
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
Why is this separate than the above pre-commit and why `2` indent instead of `4`
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
In any case, let's separate the formatting and pre-commit change to a different PR than the bugfix please
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
Thinking about this more: @msumit do you think we will be able to validate `set` or non-JSON structures?
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
Thinking about this more: @msumit do you think we will be able to validate `set` or non-JSON structures using the JSON Schema validation?
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
The following code runs jsonschema validation if one is supplied.
https://github.com/apache/airflow/blob/02b7e2c092902bf42791a5ef1a70bc71226f1e32/airflow/models/param.py#L47-L51
Example test:
https://github.com/apache/airflow/blob/02b7e2c092902bf42791a5ef1a70bc71226f1e32/tests/models/test_param.py#L58-L66
This won't be possible with "set" is what I mean:
i.e. the following works:
```python
def test_set_param(self):
p = Param({'a', 'b'})
assert p.resolve() == {'a', 'b'}
```
but there is no way of adding validation like the set only contains <10 elements, example:
```python
def test_set_param(self):
p = Param({'a', 'b'}, type='set', maxItems=1)
assert p.resolve() == {'a', 'b'}
```
**It is not a big deal but worth pointing that out in the docs**
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
The following code runs jsonschema validation if one is supplied.
https://github.com/apache/airflow/blob/02b7e2c092902bf42791a5ef1a70bc71226f1e32/airflow/models/param.py#L47-L51
Example test:
https://github.com/apache/airflow/blob/02b7e2c092902bf42791a5ef1a70bc71226f1e32/tests/models/test_param.py#L58-L66
This won't be possible with "set" is what I mean:
i.e. the following works:
```python
def test_set_param(self):
p = Param({'a', 'b'})
assert p.resolve() == {'a', 'b'}
```
but there is no way of adding validation like the set only contains <10 elements, example:
```python
def test_set_param(self):
p = Param({'a', 'b'}, type='set', maxItems=1)
assert p.resolve() == {'a', 'b'}
```
**It is not a big deal but worth pointing that out in the docs** but that's a separate issue and can be in a separate PR
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r742571324
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -1433,29 +1461,32 @@ def test_serialized_objects_are_sorted(self, object_to_serialized, expected_outp
assert serialized_obj == expected_output
def test_params_upgrade(self):
+ """when pre-2.2.0 param (i.e. primitive) is deserialized we convert to Param"""
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
- "fileloc": __file__,
+ "fileloc": '__file__',
"tasks": [],
"timezone": "UTC",
"params": {"none": None, "str": "str", "dict": {"a": "b"}},
},
}
- SerializedDAG.validate_schema(serialized)
Review comment:
fyi @jedcunningham @kaxil @msumit we have to remove this line because it is validating pre-2.2.0 params against the current schema.
it's not needed because `validate_schema` is only called when serializing; not when deserializing, and the test still verifies that the json deserializes properly.
if we wanted to keep this line, we'd have to loosen `schema.json` to allow arbitrary dict:
```json
"params": {
"anyOf": [
{ "$ref": "#/definitions/params_dict" },
{"$ref": "#/definitions/dict"}]
},
```
But this is no longer the serialization schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-960357435
@dstandish Can you take a look at the failing tests please
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738872206
##########
File path: airflow/models/param.py
##########
@@ -49,6 +49,7 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
Review comment:
I think we use `__type` in most places so this should also do for consistency.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738962350
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
that's not what's going on here
the KeyError is essentially 2.2.0 serialiazation approach, which for params, doesn't run their attrs through our serialization logic, but assumes they are json serializable (which is the problem, because in the case of sets they are not)
but so when you try to _deserialize something that was not _serialize(d), you get a KeyError and in this case we can just take the args as they are
_however_, i am working on a better approach, cleaner, and i'll push it tomorrow.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738963140
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
separately though, do we really need to support non-json-serializable params? this will cause problems, for example when you run trigger dag in the UI -- probably in that case they _must_ be valid json. so what's the point of supporting non-json in the default? i think it would be best if we simply require that params be valid json. if users want to use set operations in their templates, they can do so in the manner i demonstrated in the reported issue; and we can still keep the param values themselves json.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738963140
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
separately though, do we really need to support non-json-serializable params? this will cause problems, for example when you run trigger dag in the UI -- probably in that case they _must_ be valid json. so what's the point of supporting non-json in the default? i think it would be best if we simply require that params be valid json. if users want to use set operations in their templates, they can do so in the manner i demonstrated in the reported issue; and we can still keep the param values themselves json. @jedcunningham @kaxil interested in your thougts 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] dstandish edited a comment on pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#issuecomment-953981128
there's a bit of awkwardness with the `Param` object as relates to serialization because, while for other objects we can reuse `__dict__` as kwargs, for params, `default` is a kwarg but not an attribute. it is tempting to add `default` as an attribute perhaps make `value` a property or just remove it, but not sure of downstream consequences.
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r738963140
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
separately though, do we really need to support non-json-serializable params? this will cause problems, for example when you run trigger dag in the UI -- probably in that case they _must_ be valid json. so what's the point of supporting non-json in the default?
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r739609644
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -417,7 +417,10 @@ def _serialize_param(cls, param: Param):
@classmethod
def _deserialize_param(cls, param_dict: Dict):
param_class = import_string(param_dict.pop(Param.CLASS_IDENTIFIER))
- param_kwargs = cls._deserialize(param_dict)
+ try:
+ param_kwargs = cls._deserialize(param_dict)
+ except KeyError:
+ param_kwargs = param_dict
Review comment:
after chatting with kaxil, we need to restore support for this before properly deprecating
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741650042
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
ok just sat down to not `_serialize` the `description` attr. and on second thought, i think it's better to run it through `_serialize` after all. like, that's precisely what `_serialize` is there for. there's a reason that `_serialize` has handling for primitive types, and so we should use it; to do otherwise is essentially to duplicate serialization logic. also, not running through `_serialize` would not achieve the goal of preventing a user from using non-string values (it only would prevent from using non-json-serializable types such as `set`) -- if we really want to clamp down on types, probably the right place do do that is in `Param.__init__`.
so, i think i'll leave this as is, but let me know if you think otherwise.
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] dstandish commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r742318716
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
ok updated
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r742480649
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -325,7 +324,7 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
elif isinstance(var, TaskGroup):
return SerializedTaskGroup.serialize_task_group(var)
elif isinstance(var, Param):
- return cls._encode(var.dump(), type_=DAT.PARAM)
+ return cls._encode(cls._serialize_param(var), type_=DAT.PARAM)
Review comment:
oh, _that_ kind of validation. so yeah this is why i think we should immediately deprecate this feature (namely, support for non-json-serializable params), and perhaps warn whenever we get params with a default that is non-json-serializable.
do you think i should add such a warning to this PR?
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r743320052
##########
File path: tests/serialization/test_dag_serialization.py
##########
@@ -1433,29 +1461,32 @@ def test_serialized_objects_are_sorted(self, object_to_serialized, expected_outp
assert serialized_obj == expected_output
def test_params_upgrade(self):
+ """when pre-2.2.0 param (i.e. primitive) is deserialized we convert to Param"""
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
- "fileloc": __file__,
+ "fileloc": '__file__',
Review comment:
this value actually does not have any impact on the test. it could be `/hello/world.py`
but when you put `__file__` you can't run the bit of code interactively.
```python
serialized = {
"__version": 1,
"dag": {
"_dag_id": "simple_dag",
"fileloc": __file__,
"tasks": [],
"timezone": "UTC",
"params": {"none": None, "str": "str", "dict": {"a": "b"}},
},
}
dag = SerializedDAG.from_dict(serialized)
Traceback (most recent call last):
File "/Users/dstandish/.virtualenvs/local/lib/python3.8/site-packages/IPython/core/interactiveshell.py", line 3444, in run_code
exec(code_obj, self.user_global_ns, self.user_ns)
File "<ipython-input-2-4ee74d1eaf6b>", line 5, in <module>
"fileloc": __file__,
NameError: name '__file__' is not defined
```
--
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 #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r742256901
##########
File path: .pre-commit-config.yaml
##########
@@ -195,6 +195,15 @@ repos:
- "4"
files: ^chart/values\.schema\.json$|^chart/values_schema\.schema\.json$
pass_filenames: true
+ - id: pretty-format-json
+ name: Serialization schema
+ args:
+ - --autofix
+ - --no-sort-keys
+ - --indent
+ - "2"
Review comment:
yeah i'll split it out.
but yeah it was previously not processed in pre-commit. but it should be to ensure consistent formatting, even if that means slightly less pretty.
and yeah i just chose indent of 2 because that's what the file already had.
--
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] msumit commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741599621
##########
File path: airflow/models/param.py
##########
@@ -49,6 +49,7 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
Review comment:
In the original PR, I initially used `__type` only, but Ash felt that it should be called `__class`, cause it would be representing a Param class & not a native type, which made sense to me.
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
Do we really need to serialize/deserialize a parma's description as well? It ought to be a simple string only.
--
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] msumit commented on a change in pull request #19267: Fix serialization of Params with set data type
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #19267:
URL: https://github.com/apache/airflow/pull/19267#discussion_r741599621
##########
File path: airflow/models/param.py
##########
@@ -49,6 +49,7 @@ class Param:
"""
__NO_VALUE_SENTINEL = NoValueSentinel()
+ CLASS_IDENTIFIER = '__class'
Review comment:
In the original PR, I initially used `__type` only, but Ash felt that it should be called `__class`, cause it would be representing a Param class & not a native type, which made sense to me.
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -409,30 +406,59 @@ def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -
return True
return False
+ @classmethod
+ def _serialize_param(cls, param: Param):
+ return dict(
+ __class=f"{param.__module__}.{param.__class__.__name__}",
+ default=cls._serialize(param.value),
+ description=cls._serialize(param.description),
Review comment:
Do we really need to serialize/deserialize a parma's description as well? It ought to be a simple string only.
--
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