You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/19 07:55:26 UTC
[GitHub] [airflow] dstandish opened a new pull request, #28454: Fix bad pods pickled in executor_config
dstandish opened a new pull request, #28454:
URL: https://github.com/apache/airflow/pull/28454
We used to pickle raw pods objects but found that when unpickling across k8s lib versions we would get missing attr errors. We can fix this by roundtripping to json in a more tolerant fashion than is done by the openapi-generated code, i.e. by populating missing attrs with None.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1055696501
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
+ }
Review Comment:
actually, no @uranusjr filtering out None objects is intentional and the current behavior. my only change is to tolerate when the attr is missing. that's all i want to change, otherwise it should be identical.
--
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 diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1052414950
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
+ }
Review Comment:
That uses pod generator which uses the kubernetes client method that we are duplicating and fixing here
--
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 diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1052426654
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
+ }
Review Comment:
I considered maybe just replacing that call with this logic but, I figure, probably best in general to just lean on kubernetes code and only vendor in and fix this but for backcompat when unpicking
In face probably should also test that to dic fails before running through to reduce usage of it further
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #28454:
URL: https://github.com/apache/airflow/pull/28454#issuecomment-1357242313
Cc @bolkedebruin who has just done a lot of work on serialisation code in Airflow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish merged PR #28454:
URL: https://github.com/apache/airflow/pull/28454
--
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 diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1051909915
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +158,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
Review Comment:
this is copied from https://github.com/kubernetes-client/python/blob/master/kubernetes/client/api_client.py#L202
i'll point out the only differenc below (line 203)
--
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] dimberman commented on a diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dimberman commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1055774132
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
+ }
+
+ return {key: sanitize_for_serialization(val) for key, val in obj_dict.items()}
+
+
+def ensure_pod_is_valid_after_unpickling(pod: V1Pod) -> V1Pod | None:
+ """
+ Convert pod to json and back so that pod is safe.
Review Comment:
Can you explain what you mean by "safe" in this docstring?
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
Review Comment:
Can you separate this out into a a function? Might make it easier to test/read in the future.
##########
airflow/utils/sqlalchemy.py:
##########
@@ -188,9 +260,20 @@ def process(value):
if isinstance(value, dict) and "pod_override" in value:
pod_override = value["pod_override"]
- # If pod_override was serialized with Airflow's BaseSerialization, deserialize it
if isinstance(pod_override, dict) and pod_override.get(Encoding.TYPE):
+ # If pod_override was serialized with Airflow's BaseSerialization, deserialize it
value["pod_override"] = BaseSerialization.deserialize(pod_override)
+ else:
+ # backcompat path
+ # pod_override is a V1Pod object. re-serialize it to ensure it is
+ # not going to blow up (objects created with one k8s version, when
+ # unpickled in an env with upgraded k8s version, may blow up when
+ # `to_dict` is called, because openapi client code gen calls
+ # getattr on all attrs in openapi_types for each object, and when
+ # new attrs are added to that list, getattr will fail.
Review Comment:
Oh that is terrifying.
--
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 #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28454:
URL: https://github.com/apache/airflow/pull/28454#issuecomment-1357234672
thoughts
should we instead (partially) replace this serialize method and use it?
https://github.com/apache/airflow/blob/b52ce7f483d02fe17467ca3fa13cfffdb92ac103/airflow/kubernetes/pod_generator.py#L434
should we add warnings?
should we set to None when the attempt to round trip fails?
do we need to check for v1pod... i don't think 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] dstandish commented on a diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1055692417
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
+ }
Review Comment:
> Doe this work? (most relevant change is the hasattr use) I think the current getattr may incorrectly filter out None values
i will try it @uranusjr
--
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] dimberman commented on pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dimberman commented on PR #28454:
URL: https://github.com/apache/airflow/pull/28454#issuecomment-1364153120
@dstandish go for 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 diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1051910861
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +158,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, PRIMITIVE_TYPES):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
Review Comment:
only difference is this line, where we have a default of None
compare with [here](https://github.com/kubernetes-client/python/blob/5a96bbcbe21a552cc1f9cda13e0522fafb0dbac8/kubernetes/client/api_client.py#L239).
--
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 diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1052121208
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
+ }
Review Comment:
```suggestion
attrs = (attr for attr in obj.openapi_types if hasattr(obj, attr))
obj_dict = {
obj.attribute_map[attr]: getattr(obj, attr)
for attr in attrs
}
```
Doe this work? (most relevant change is the `hasattr` use) I think the current `getattr` may incorrectly filter out None values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
kaxil commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1052192261
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
+ }
Review Comment:
Can we not use https://github.com/apache/airflow/blob/main/airflow/serialization/serializers/kubernetes.py to not have duplication?
--
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 diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1055844290
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
Review Comment:
i would prefer not to. the point is that we don't want to do anythnig different from the k8s code... *except* allow missing attr.
if we go making changes and refactoring, that only increases the chance that we diverge from the behavior of k8s, and makes it impossible to compare against the k8s code.
--
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 diff in pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28454:
URL: https://github.com/apache/airflow/pull/28454#discussion_r1055854652
##########
airflow/utils/sqlalchemy.py:
##########
@@ -153,6 +156,75 @@ def process_result_value(self, value, dialect):
return BaseSerialization.deserialize(value)
+def sanitize_for_serialization(obj: V1Pod):
+ """
+ Convert pod to dict.... but *safely*.
+
+ When pod objects created with one k8s version are unpickled in a python
+ env with a more recent k8s version (in which the object attrs may have
+ changed) the unpickled obj may throw an error because the attr
+ expected on new obj may not be there on the unpickled obj.
+
+ This function still converts the pod to a dict; the only difference is
+ it populates missing attrs with None.
+
+ If obj is None, return None.
+ If obj is str, int, long, float, bool, return directly.
+ If obj is datetime.datetime, datetime.date
+ convert to string in iso8601 format.
+ If obj is list, sanitize each element in the list.
+ If obj is dict, return the dict.
+ If obj is OpenAPI model, return the properties dict.
+
+ :param obj: The data to serialize.
+ :return: The serialized form of data.
+
+ :meta private:
+ """
+ if obj is None:
+ return None
+ elif isinstance(obj, (float, bool, bytes, str, int)):
+ return obj
+ elif isinstance(obj, list):
+ return [sanitize_for_serialization(sub_obj) for sub_obj in obj]
+ elif isinstance(obj, tuple):
+ return tuple(sanitize_for_serialization(sub_obj) for sub_obj in obj)
+ elif isinstance(obj, (datetime.datetime, datetime.date)):
+ return obj.isoformat()
+
+ if isinstance(obj, dict):
+ obj_dict = obj
+ else:
+ obj_dict = {
+ obj.attribute_map[attr]: getattr(obj, attr)
+ for attr, _ in obj.openapi_types.items()
+ if getattr(obj, attr, None) is not None
Review Comment:
i made this clearer in the code through docstring / comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #28454: Fix bad pods pickled in executor_config
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #28454:
URL: https://github.com/apache/airflow/pull/28454#issuecomment-1357403398
I’m wondering is there’s a way that doesn’t duplicate so much code. For example, maybe we could recursively fix up the dict directly before feeding it into `kubernetes`?
--
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