Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default json encoder serialization in Task SDK logging #45962

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

ianbuss
Copy link
Contributor

@ianbuss ianbuss commented Jan 23, 2025

When presented with an non-stdlib object the current msgspec JSON encoder used by the Task SDK throws an ugly exception and causes the supervisor to crash. This can happen for example if presented with a Pydantic class such as is used between the supervisor and the API server.

This PR makes a very targeted change to provide the default encoding function to the msgspec encoder if it is supplied.
A test is added to ensure the JSON serialization works when a Pydantic class is supplied.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe see if we can remove msgspec dep now? LGTM otherwise

Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One qn, rest looks good

@ianbuss ianbuss changed the title Use orjson for json serialization in Task SDK logging Fix default json encoder serialization in Task SDK logging Jan 28, 2025
@feluelle
Copy link
Member

Merging as the failed mypy check is unrelated.

@feluelle feluelle merged commit 2c1a3cc into apache:main Jan 29, 2025
61 of 62 checks passed
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
)

* Use orjson for json serialization in Task SDK logging

* Formatting fixes

* Use msgspec enc_hook

* Sort pyproject dependencies
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Jan 30, 2025
)

* Use orjson for json serialization in Task SDK logging

* Formatting fixes

* Use msgspec enc_hook

* Sort pyproject dependencies
niklasr22 pushed a commit to niklasr22/airflow that referenced this pull request Feb 8, 2025
)

* Use orjson for json serialization in Task SDK logging

* Formatting fixes

* Use msgspec enc_hook

* Sort pyproject dependencies
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 17, 2025
)

* Use orjson for json serialization in Task SDK logging

* Formatting fixes

* Use msgspec enc_hook

* Sort pyproject dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants