ferruzzi commented on code in PR #23628:
URL: https://github.com/apache/airflow/pull/23628#discussion_r870549456
##########
airflow/providers/amazon/aws/operators/sagemaker.py:
##########
@@ -188,7 +193,7 @@ def execute(self, context: 'Context') -> dict:
)
if response['ResponseMetadata']['HTTPStatusCode'] != 200:
raise AirflowException(f'Sagemaker Processing Job creation failed:
{response}')
- return {'Processing':
self.hook.describe_processing_job(self.config['ProcessingJobName'])}
+ return {'Processing':
serialize(self.hook.describe_processing_job(self.config['ProcessingJobName']))}
Review Comment:
I can move it to serialize the whole return value if you'd prefer. My
thinking was that this way it would still return a dict, albeit a dict with one
key and one value instead of the full dict, and I wasn't changing the return
type contract.
Fixing the XCOM serialization at the source would be a better answer if you
think that is suitable and if other provider packages are running into this.
It seemed excessive if the problem wasn't being seen elsewhere, and I didn't
remember hearing about anyone else running into it so I decided to keep the fix
as local as possible.
You said serializing just the datetime objects in place was more hacky, but
honestly I like the idea. It would solve the problem while keeping the
original dict structure. If we don't go with fixing the XCOM directly, I think
that is a better solution that what I have here.
I started [a
thread](https://apache-airflow.slack.com/archives/CCPRP7943/p1652288632845719)
on the Slack to discuss it with the community.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
