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

Replace old Breeze with Python based implementation #22880

Merged
merged 1 commit into from
Apr 10, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Apr 9, 2022

Over the last few months together with Outreachy interns
we rewrote the most important functionality of the old Bash-based
Breeze with Python Based implementation.

We approached it in systematic way with capturing all our decisions
in the ADR format (dev/breeze/docs) and implementing the parts
that are used on a daily basis by the users. Breeze2 as it was
called is ready for Prime-Time with the users so we are swapping
out the old breeze wiht the new one.

The old breeze has been moved to breeze-legacy and we will
gradually parts of it that are already migrated to Python and proven
while continue rewriting the parts that are missing (mostly the
maintainer tools) and replacing the remaining CI shell scripts with
the new breeze commands.

We also need to make sure that there is no accidental top-level
import for extra packages added in the future, because people
who installed breeze before will not have it - so we have
a pre-commit that checks if breeze.py can be parsed and
--help executed with just rich and click installed.

This PR:

  • moves breeze to breeze-legacy
  • moves Breeze2 to breeze
  • updates documentation and screenshots where applicable
  • explains old vs. new breeze in documentation
  • adds protection so that no accidental top-level import is
    added to breeze.py and files imported from there

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@potiuk
Copy link
Member Author

potiuk commented Apr 9, 2022

cc: @Bowrna @edithturn

@potiuk
Copy link
Member Author

potiuk commented Apr 9, 2022

Here it is.

Switching to Python based Breeze. FINALLY.

Note. I would like to drag your attention to this nicely generated screenshots in our Breeze documentation :) https://github.com/potiuk/airflow/blob/swap-old-breeze-with-new/BREEZE.rst#installation

@potiuk potiuk requested a review from turbaszek April 9, 2022 19:04
@edithturn
Copy link
Contributor

@potiuk I ran it on my machine (Linux Ubuntu 20.10), wow works very nice! That part of the comments with a click is fantastic 🤩

I ran some commands like "config" and "start-airflow". My question is if all these options al already implemented.

  • In config, I got no output or a warning saying that we set Python as a backend. (Maybe I don't understand exactly how is its behavior)
  • I also ran, start-airflow, and tmux session is being open on my VSC terminal, (this is so nice 👏🏼 ), my question is in the part of creating permissions (four windows). Am I doing something wrong to get that error? or I should do an extra step to have it configure it
    Screenshot from 2022-04-09 19-07-12

It is fantastic @potiuk and also is 💯 faster on my computer 👏🏼 🚀

@potiuk
Copy link
Member Author

potiuk commented Apr 10, 2022

  • In config, I got no output or a warning saying that we set Python as a backend. (Maybe I don't understand exactly how is its behavior)

Good Point. We should give feedback to the user ! The idea is that when you set python version, next time you get that default python version when you do not specify any python version. I.e. .

breeze -> Python 3.7 used by default
breeze config --python 3.8 -> python 3.8 set as default
breeze -> Python 3.8 used

I also ran, start-airflow, and tmux session is being open on my VSC terminal, (this is so nice 👏🏼 ), my question is in the part of creating permissions (four windows). Am I doing something wrong to get that error? or I should do an extra step to have it configure it

The errors are unrelated - they are also with current breeze' start-airflow - I will raise that to others as we are very close to cutting 2.3 branch I think those errors need to be fixed before.

@potiuk potiuk force-pushed the swap-old-breeze-with-new branch 2 times, most recently from 37d43fc to 97fd8d7 Compare April 10, 2022 09:03
@potiuk potiuk changed the title Replaces old Breeze with Python based implementation Replace old Breeze with Python based implementation Apr 10, 2022
@potiuk potiuk force-pushed the swap-old-breeze-with-new branch from 97fd8d7 to ba33a97 Compare April 10, 2022 09:05
@potiuk
Copy link
Member Author

potiuk commented Apr 10, 2022

Config now is more "obvious". It also prints summary of the config (@edithturn ):

Screenshot 2022-04-10 at 11 11 36

@potiuk potiuk force-pushed the swap-old-breeze-with-new branch 3 times, most recently from d18230f to 8c9fd0d Compare April 10, 2022 11:12
@potiuk
Copy link
Member Author

potiuk commented Apr 10, 2022

I've also added protection to not add accidentally new top-level imports in breeze.py as they would break auto-complete and self-upgrade.

@potiuk potiuk force-pushed the swap-old-breeze-with-new branch 5 times, most recently from 268431e to 77e9da0 Compare April 10, 2022 11:34
@Bowrna
Copy link
Contributor

Bowrna commented Apr 10, 2022

@potiuk this is amazing :) you have made this change very quickly 👍

@potiuk potiuk mentioned this pull request Apr 10, 2022
@potiuk potiuk force-pushed the swap-old-breeze-with-new branch from 77e9da0 to 9d91960 Compare April 10, 2022 12:36
@potiuk potiuk force-pushed the swap-old-breeze-with-new branch 5 times, most recently from ded482c to c3cda62 Compare April 10, 2022 14:43
@potiuk
Copy link
Member Author

potiuk commented Apr 10, 2022

Looks like Green and ready for Python's Breeze prime time!

@edithturn
Copy link
Contributor

Go ahead @potiuk, let us know! 🚀 💪🏼

Over the last few months together with Outreachy interns
we rewrote the most important functionality of the old Bash-based
Breeze with Python Based implementation.

We approached it in systematic way with capturing all our decisions
in the ADR format (dev/breeze/docs) and implementing the parts
that are used on a daily basis by the users. Breeze2 as it was
called is ready for Prime-Time with the users so we are swapping
out the old breeze wiht the new one.

The old `breeze` has been moved to `breeze-legacy` and we will
gradually parts of it that are already migrated to Python and proven
while continue rewriting the parts that are missing (mostly the
maintainer tools) and replacing the remaining CI shell scripts with
the new `breeze` commands.

We also need to make sure that there is no accidental top-level
import for extra packages added in the future, because people
who installed breeze before will not have it - so we have
a pre-commit that checks if breeze.py can be parsed and
--help executed with just rich and click installed.

This PR:

* moves `breeze` to `breeze-legacy`
* moves `Breeze2` to `breeze`
* updates documentation and screenshots where applicable
* explains old vs. new breeze in documentation
* adds protection so that no accidental top-level import is
  added to breeze.py and files imported from there

Fixes: apache#22827
@potiuk potiuk force-pushed the swap-old-breeze-with-new branch from c3cda62 to 75cc8e8 Compare April 10, 2022 15:43
@potiuk potiuk requested a review from eladkal April 10, 2022 17:27
@potiuk
Copy link
Member Author

potiuk commented Apr 10, 2022

Hey @eladkal :). That's IT..

@potiuk potiuk linked an issue Apr 10, 2022 that may be closed by this pull request
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

🎉

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Apr 10, 2022
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk potiuk merged commit 1007828 into apache:main Apr 10, 2022
@potiuk potiuk deleted the swap-old-breeze-with-new branch April 10, 2022 18:47
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breeze: Update documentation for Breeze2
5 participants