Skip to content

Unify selecting constraints option when installing airflow #52274

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

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 26, 2025

Due to the way how it historically got added - we had two ways of selecting whether we are installing airlfow dyanmically in breeze with or without constraints:

  • --install-airflow-with-constraints - was used in a few places
  • --skip-airflow-constraints - was used in other places

The logic to handle those were broken at places where they contradicted each other. This PR unifies it and only uses the --install-airflow-with-constraints flag in all the places where we need to determine whether constraints are used or not and it fixes the logic.

The logic of installation had been reviewed, refactored into separate methods doing smaller tasks and more diagnostics was added.


^ 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 airflow-core/newsfragments.

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Jun 26, 2025
@boring-cyborg boring-cyborg bot added area:dev-tools backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch labels Jun 26, 2025
@potiuk potiuk requested a review from eladkal June 26, 2025 06:18
@potiuk
Copy link
Member Author

potiuk commented Jun 26, 2025

I will have to test it also with "upgrade with newer dependencies" and "canary" before I merge it.

@potiuk
Copy link
Member Author

potiuk commented Jun 26, 2025

Ok - now I have consistent at least (and failing) - and can fix it :)

Due to the way how it historically got added - we had two ways of
selecting whether we are installing airlfow dyanmically in breeze
with or without constraints:

* --install-airflow-with-constraints - was used in a few places
* --skip-airflow-constraints - was used in other places

The logic to handle those were broken at places where they
contradicted each other. This PR unifies it and only uses
the --install-airflow-with-constraints flag in all the places
where we need to determine whether constraints are used or not
and it fixes the logic.

The logic of installation had been reviewed, refactored into
separate methods doing smaller tasks and more diagnostics was
added.
@potiuk potiuk force-pushed the fix-install-airflow-with-constraints-logic branch from 3ca47a0 to 4474d0f Compare June 26, 2025 07:52
@potiuk
Copy link
Member Author

potiuk commented Jun 26, 2025

I refactored the installation script and split the long and complex method into smaller, separate methods doing things in much easiert to understand way.

@potiuk
Copy link
Member Author

potiuk commented Jun 26, 2025

Let me try with upgrade

@potiuk potiuk added the upgrade to newer dependencies If set, upgrade to newer dependencies is forced label Jun 26, 2025
@potiuk potiuk closed this Jun 26, 2025
@potiuk potiuk reopened this Jun 26, 2025
Copy link
Member

@gopidesupavan gopidesupavan left a comment

Choose a reason for hiding this comment

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

Thanks Nice removal :)

@potiuk
Copy link
Member Author

potiuk commented Jun 26, 2025

Indeed seems like it is going to be easier to understand it now - also for me :)

@potiuk potiuk merged commit 8846bef into apache:main Jun 26, 2025
194 of 195 checks passed
@potiuk potiuk deleted the fix-install-airflow-with-constraints-logic branch June 26, 2025 10:27
Copy link

Backport failed to create: v3-0-test. View the failure log Run details

Status Branch Result
v3-0-test Commit Link

You can attempt to backport this manually by running:

cherry_picker 8846bef v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

potiuk added a commit to potiuk/airflow that referenced this pull request Jun 26, 2025
apache#52274)

Due to the way how it historically got added - we had two ways of
selecting whether we are installing airlfow dyanmically in breeze
with or without constraints:

* --install-airflow-with-constraints - was used in a few places
* --skip-airflow-constraints - was used in other places

The logic to handle those were broken at places where they
contradicted each other. This PR unifies it and only uses
the --install-airflow-with-constraints flag in all the places
where we need to determine whether constraints are used or not
and it fixes the logic.

The logic of installation had been reviewed, refactored into
separate methods doing smaller tasks and more diagnostics was
added.
(cherry picked from commit 8846bef)

Co-authored-by: Jarek Potiuk <[email protected]>
potiuk added a commit to potiuk/airflow that referenced this pull request Jun 26, 2025
apache#52274)

Due to the way how it historically got added - we had two ways of
selecting whether we are installing airlfow dyanmically in breeze
with or without constraints:

* --install-airflow-with-constraints - was used in a few places
* --skip-airflow-constraints - was used in other places

The logic to handle those were broken at places where they
contradicted each other. This PR unifies it and only uses
the --install-airflow-with-constraints flag in all the places
where we need to determine whether constraints are used or not
and it fixes the logic.

The logic of installation had been reviewed, refactored into
separate methods doing smaller tasks and more diagnostics was
added.
(cherry picked from commit 8846bef)

Co-authored-by: Jarek Potiuk <[email protected]>
potiuk added a commit to potiuk/airflow that referenced this pull request Jun 26, 2025
apache#52274)

Due to the way how it historically got added - we had two ways of
selecting whether we are installing airlfow dyanmically in breeze
with or without constraints:

* --install-airflow-with-constraints - was used in a few places
* --skip-airflow-constraints - was used in other places

The logic to handle those were broken at places where they
contradicted each other. This PR unifies it and only uses
the --install-airflow-with-constraints flag in all the places
where we need to determine whether constraints are used or not
and it fixes the logic.

The logic of installation had been reviewed, refactored into
separate methods doing smaller tasks and more diagnostics was
added.
(cherry picked from commit 8846bef)

Co-authored-by: Jarek Potiuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch full tests needed We need to run full set of tests for this PR to merge upgrade to newer dependencies If set, upgrade to newer dependencies is forced
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants