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

Proposal for different fix to 2.10 regression #1163

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

dwreeves
Copy link
Collaborator

@dwreeves dwreeves commented Aug 19, 2024

Description

Fixes Airflow 2.10 regression / compat

Related Issue(s)

This is a fix for #1161, and is designed to be a different proposal to the changes in #1162.

Ultimately the issue that popped up in 2.10 relates to how the MRO was impacted by the Mixin class.

The simplest, most robust fix to make the MRO work properly is to make each mixin a subclass of BaseOperator. This doesn't require the unintuitive hack of needing an __init__() call to shuffle the MRO around, and it makes things more easily maintainable going forward, I think.

A note: This PR is merging to test-af-2.10, not main.

Tested for 2.10.

I also removed an unnecessary __init__ for the DbtTestMixin while I was at it.

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc labels Aug 19, 2024
@dwreeves dwreeves requested a review from pankajastro August 19, 2024 02:11
@dwreeves dwreeves changed the title test af 2.10 Proposal for different fix to 2.10 regression Aug 19, 2024
@dwreeves
Copy link
Collaborator Author

One more thing, after all of this gets resolved, we should either do this for each minor version from 1.3+, or alternatively put an upper bound on each previously released version as patch releases. I believe based on the Slack that there are a lot of users on 1.3 still.

Copy link
Contributor

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

Looks good. thanks @dwreeves for looking into.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 19, 2024
@pankajastro
Copy link
Contributor

cc: @pankajkoti we should include this in 1.6.0

@pankajastro
Copy link
Contributor

pankajastro commented Aug 19, 2024

we should either do this for each minor version from 1.3+, or alternatively put an upper bound on each previously released version as patch releases.

I'm not sure about this at the moment, but I'm open to hearing what others think. The user who has reported this issue using Cosmos 1.5.1.
Update: We may not need to patch it

  • This error happens only for Airflow 2.10 and I believe users should upgrade Cosmos too
  • I think we do not follow the practice of backporting fixes for this project?

I believe based on the Slack that there are a lot of users on 1.3 still.

Which Slack are you referring to?

@pankajastro
Copy link
Contributor

Also, the CI is not running on this PR, possibly because the base branch is not main.

@pankajastro
Copy link
Contributor

I'm merging this to branch test-af-2.10 to validate the CI check

@pankajastro pankajastro merged commit e6bc8de into astronomer:test-af-2.10 Aug 19, 2024
1 check passed
@pankajastro
Copy link
Contributor

Look like CI is not happy

@dwreeves
Copy link
Collaborator Author

@pankajastro That's weird. 😕 I did run tests locally for 2.10 and it worked, but I see now that 2.4 tests break.

Let's rollback on the other PR and we can merge this if/when it is ready. There is a higher criticality to just releasing a fix than there is to fixing it in a "good way."

Sorry for making this so complicated.

@dwreeves
Copy link
Collaborator Author

dwreeves commented Aug 19, 2024

RE: upgrading other versions-- Fair enough, we won't backport then.

My one concern was that Airflow issues like this can be annoying because if a user upgrades their Airflow, downgrading Airflow to rollback a change can be very annoying because of the Postgres db requiring a downgrade, so if they are stuck on an old Cosmos version for some reason and they upgrade Airflow, now they have 2 problems (2.10 broke things for them + they cannot easily downgrade, but also, Cosmos cannot be upgraded either).

Which Slack are you referring to?

Official Airflow slack, dbt channel.

@tatiana tatiana added this to the Cosmos 1.7.0 milestone Sep 25, 2024
@pankajastro pankajastro mentioned this pull request Nov 18, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants