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

Restrict marshmallow==3.23 #45442

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

gopidesupavan
Copy link
Member

It looks like marshmallow==3.24 introduced the breaking changes to
https://github.com/marshmallow-code/marshmallow/blob/dev/CHANGELOG.rst#3240-2025-01-06

Field <marshmallow.fields.Field>, Mapping <marshmallow.fields.Mapping>, and Number <marshmallow.fields.Number> should no longer be used as fields within schemas. Use their subclasses instead.
Likely this PR marshmallow-code/marshmallow#2723

We use <marshmallow.fields.Number> in https://github.com/apache/airflow/blob/main/airflow/api_connexion/schemas/task_schema.py#L51. Likely these updates required?

CI is failing https://github.com/apache/airflow/actions/runs/12639019672/job/35217178280#step:6:4004


^ 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
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.

Thanks!

@amoghrajesh
Copy link
Contributor

The failure seems to be due to flakiness, retriggered.

@amoghrajesh amoghrajesh merged commit 8639ade into apache:main Jan 7, 2025
156 checks passed
@amoghrajesh amoghrajesh changed the title Restrict marshmallow==3.24 Restrict marshmallow==3.23 Jan 7, 2025
@potiuk
Copy link
Member

potiuk commented Jan 7, 2025

Nice!

@emorikawa
Copy link

@gopidesupavan thank you for finding and fixing this. The effect of marshmallow's breaking change was significant for me

Since this does schema validation, this effectively caused all of my Airflow API requests to suddenly start 400'ing on my next deploy (deploys re pip-install things when the container is rebuilt).

I have business critical functions that depend on programatically launching DAGs. All of those broke suddenly.

@sloria
Copy link
Contributor

sloria commented Jan 8, 2025

marshmallow 3.24 doesn't actually have a breaking change: it only raises warnings for usages that are changed or removed in 4.0.

The issue is that marshmallow-sqlalchemy (a dependency of Flask-AppBuilder) needs to be upgraded to version 1.1.1 to work correctly with marshmallow. So I think this should be fixed by relaxing the version constraint here: https://github.com/dpgaspar/Flask-AppBuilder/blob/418ab8a93907669be4ccbb99d7aefa5283f3e013/setup.py#L61

Once I get my Flask-AppBuilder dev environment set up, I can send a PR with that change

@potiuk
Copy link
Member

potiuk commented Jan 8, 2025

The issue is that marshmallow-sqlalchemy (a dependency of Flask-AppBuilder) needs to be upgraded to version 1.1.1 to work correctly with marshmallow. So I think this should be fixed by relaxing the version constraint here: https://github.com/dpgaspar/Flask-AppBuilder/blob/418ab8a93907669be4ccbb99d7aefa5283f3e013/setup.py#L61

Nice find . Thanks for following it up!

@sloria
Copy link
Contributor

sloria commented Jan 8, 2025

i just released marshmallow 3.24.2, which fixes compatibility with older marshmallow-sqlalchemy. so we no longer need to wait for Flask-AppBuilder. #45499 reverts this as it's no longer be necessary to pin marshmallow

@potiuk
Copy link
Member

potiuk commented Jan 8, 2025

i just released marshmallow 3.24.2, which fixes compatibility with older marshmallow-sqlalchemy. so we no longer need to wait for Flask-AppBuilder. #45499 reverts this as it's no longer be necessary to pin marshmallow

Cool. I rebased the PR and added legacy api label - let's see if all tests pass :)

agupta01 pushed a commit to agupta01/airflow that referenced this pull request Jan 13, 2025
HariGS-DB pushed a commit to HariGS-DB/airflow that referenced this pull request Jan 16, 2025
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants