Finish refactoring of breeze-dependent pre-commits to use breeze shell #35830

potiuk · 2023-11-24T02:39:30Z

ome of our pre-commits were using code from Breeze in rather complex
way - by inserting PYTHONPATH and importing code from there. That was
complex and brittle and with recent changes of ShelParam #35801 those
precommits required more and more dependencies to be added to their
pre-commit virtualenvs.

The reason that it was done this way was the assumption that someone
might want to run pre-commits locally without having breeze installed,
but this assumption and use case is rather unlikely, becasue breeze
becomes more and more useful and used so we can safely assume that
anyone who wants to do pre-commits will also have breeze installed and
on path. And anyway to run those pre-commits you need to have breeze
CI image pulled and built, so you should generally have breeze to run
them.

This PR finishes the series of PRs implementing the refactor and
completes the refactor.

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

eladkal

As a bonus MyPy output is now coloured (also when run as part of git commit).

cool!

jscheffl

Uff, I was looking over this and needed to make a break after ~15min of reading. This PR is quite complex and changes a lot of things in one batch.
I would have preferred to split the refactoring and changes into separate commits to be able to better understand the features or even make rather (3?) smaller PRs.
But unfortunately it is all a single commit.

Before shooting in the wild, can you help (the reviewer) a bit what you have tested and what specifically during review we should take a look for? Critical things to test to ensure it does not only "work on my machine"?

potiuk · 2023-11-25T11:51:33Z

Sure - yeah. good idea. I will split it. It grew quite a bit more than initial refactor, but I think I can split it to logical chunks.

It started from the idea that it will be a "small refactor" but when I did it, I found out that I need to add a lot more options tobreeze shell command to make it possible for pre-commits to use it. But I think I can attempt to split it to more logical separate chunks.

Let me turn it into a draft and start separating out things.

potiuk · 2023-11-25T11:52:34Z

That is also a good idea as a "learning" exercise for those who will be reviewing it - i will try to turn it into more of a breeze tutorial explaining those changes in more detail

potiuk · 2023-11-26T14:08:39Z

As discussed with @jscheffl -> the "breeze command flag update" separated part ot of this PR has been moved to #35862 . I will rebase this one on top of it.

potiuk · 2023-11-26T19:40:56Z

OK. Merged #35862 so made it ready for review.

potiuk · 2023-11-26T19:43:14Z

BTW. If this is still too big I am happy to move pre-commit by pre-commit separately .

potiuk · 2023-11-26T22:02:54Z

I started to separate things out #35873

jscheffl

I see one problem during review and actually I see a "real" problem that the pipeline in your run did not turn red. Because in my local run it failed in pre-commit run -a in:

scripts/ci/pre_commit/pre_commit_update_er_diagram.py:31: error: Argument
"project_name" to "run_command_via_breeze_shell" has incompatible type "None";
expected "str"  [arg-type]
        project_name=None,
                     ^~~~
Found 1 error in 1 file (checked 205 source files)
Error 1 returned

...and doing the manual check I can confirm that the (not changed) ER update script sets a None as parameter whereas the utils function requires a str as type.
Why has this not been catched in your CI run? Actually the CI also executes with breeze static-checks --all-files - is there another (hidden) bug that this was not blocking?

scripts/ci/pre_commit/common_precommit_utils.py

potiuk · 2023-11-27T16:28:15Z

OK. only one last pre-commit (and removal of old code) will remain here once we merge #35878

potiuk · 2023-11-27T16:48:24Z

This is what's left :) ... Should be small to complete the change :)

potiuk · 2023-11-27T16:49:08Z

dev/breeze/src/airflow_breeze/utils/run_utils.py

@@ -382,28 +380,6 @@ def check_if_image_exists(image: str) -> bool:
    return cmd_result.returncode == 0


-def get_ci_image_for_pre_commits() -> str:


Not needed any more after this last PR from the series .

Some of our pre-commits were using code from Breeze in rather complex way - by inserting PYTHONPATH and importing code from there. That was complex and brittle and with recent changes of ShelParam apache#35801 those precommits required more and more dependencies to be added to their pre-commit virtualenvs. The reason that it was done this way was the assumption that someone might want to run pre-commits locally without having breeze installed, but this assumption and use case is rather unlikely, becasue breeze becomes more and more useful and used so we can safely assume that anyone who wants to do pre-commits will also have breeze installed and on path. And anyway to run those pre-commits you need to have breeze CI image pulled and built, so you should generally have breeze to run them. This PR finishes the series of PRs implementing the refactor and completes the refactor.

jscheffl

Thanks! LGTM!

#35830) Some of our pre-commits were using code from Breeze in rather complex way - by inserting PYTHONPATH and importing code from there. That was complex and brittle and with recent changes of ShelParam #35801 those precommits required more and more dependencies to be added to their pre-commit virtualenvs. The reason that it was done this way was the assumption that someone might want to run pre-commits locally without having breeze installed, but this assumption and use case is rather unlikely, becasue breeze becomes more and more useful and used so we can safely assume that anyone who wants to do pre-commits will also have breeze installed and on path. And anyway to run those pre-commits you need to have breeze CI image pulled and built, so you should generally have breeze to run them. This PR finishes the series of PRs implementing the refactor and completes the refactor. (cherry picked from commit 8a67e15)

potiuk requested review from ashb, jedcunningham and kaxil as code owners November 24, 2023 02:39

boring-cyborg bot added area:dev-tools kind:documentation labels Nov 24, 2023

potiuk requested review from hussein-awala, utkarsharma2, jscheffl, Taragolis and amoghrajesh and removed request for ashb, kaxil and jedcunningham November 24, 2023 02:39

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch 2 times, most recently from 87d1c6b to 4f68a1e Compare November 25, 2023 01:41

eladkal approved these changes Nov 25, 2023

View reviewed changes

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch from 4f68a1e to be2a572 Compare November 25, 2023 10:35

potiuk mentioned this pull request Nov 25, 2023

add guideline for missing image when running breeze #35813

Closed

jscheffl reviewed Nov 25, 2023

View reviewed changes

potiuk marked this pull request as draft November 25, 2023 11:52

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch 4 times, most recently from 56b210b to 95de8ba Compare November 26, 2023 12:21

potiuk mentioned this pull request Nov 26, 2023

Modify breeze commands and flags for standalone shell commands #35862

Merged

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch 2 times, most recently from 5105105 to 5df59ee Compare November 26, 2023 14:14

potiuk marked this pull request as ready for review November 26, 2023 19:40

jscheffl requested changes Nov 26, 2023

View reviewed changes

scripts/ci/pre_commit/common_precommit_utils.py Outdated Show resolved Hide resolved

jscheffl mentioned this pull request Nov 26, 2023

Remove MSSQL support from Airflow core #35868

Closed

potiuk marked this pull request as draft November 27, 2023 01:11

potiuk mentioned this pull request Nov 27, 2023

Add common code and utils to implement ER pre-commit in a clearer way #35875

Merged

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch 3 times, most recently from 04e5959 to 7ee1bf4 Compare November 27, 2023 02:42

This was referenced Nov 27, 2023

Move check provider yaml file to breeze shell #35879

Merged

Move breeze command line pre-commit check to breeze shell #35881

Merged

Move pre-commit for local-file-mounts to breeze shell #35878

Merged

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch from 7ee1bf4 to 2dcd210 Compare November 27, 2023 16:25

potiuk requested a review from jscheffl November 27, 2023 16:28

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch from 2dcd210 to e611334 Compare November 27, 2023 16:46

potiuk marked this pull request as ready for review November 27, 2023 16:46

potiuk changed the title ~~Refactor and simplify breeze-based pre-commits~~ Finish refactoring of breeze-dependent pre-commits to use breeze shell Nov 27, 2023

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch from e611334 to c047a9a Compare November 27, 2023 16:48

potiuk commented Nov 27, 2023

View reviewed changes

potiuk force-pushed the refactor-and-simplify-breeze-bound-pre-commits branch from c047a9a to 0a1e6fa Compare November 27, 2023 17:05

jscheffl approved these changes Nov 27, 2023

View reviewed changes

potiuk merged commit 8a67e15 into apache:main Nov 27, 2023
72 checks passed

potiuk deleted the refactor-and-simplify-breeze-bound-pre-commits branch November 27, 2023 22:28

ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish refactoring of breeze-dependent pre-commits to use breeze shell #35830

Finish refactoring of breeze-dependent pre-commits to use breeze shell #35830

potiuk commented Nov 24, 2023 •

edited

Loading

eladkal left a comment

jscheffl left a comment

potiuk commented Nov 25, 2023

potiuk commented Nov 25, 2023

potiuk commented Nov 26, 2023

potiuk commented Nov 26, 2023

potiuk commented Nov 26, 2023

potiuk commented Nov 26, 2023

jscheffl left a comment •

edited

Loading

potiuk commented Nov 27, 2023

potiuk commented Nov 27, 2023

potiuk Nov 27, 2023

jscheffl left a comment

		@@ -382,28 +380,6 @@ def check_if_image_exists(image: str) -> bool:
		return cmd_result.returncode == 0


		def get_ci_image_for_pre_commits() -> str:

Finish refactoring of breeze-dependent pre-commits to use breeze shell #35830

Finish refactoring of breeze-dependent pre-commits to use breeze shell #35830

Conversation

potiuk commented Nov 24, 2023 • edited Loading

eladkal left a comment

Choose a reason for hiding this comment

jscheffl left a comment

Choose a reason for hiding this comment

potiuk commented Nov 25, 2023

potiuk commented Nov 25, 2023

potiuk commented Nov 26, 2023

potiuk commented Nov 26, 2023

potiuk commented Nov 26, 2023

potiuk commented Nov 26, 2023

jscheffl left a comment • edited Loading

Choose a reason for hiding this comment

potiuk commented Nov 27, 2023

potiuk commented Nov 27, 2023

potiuk Nov 27, 2023

Choose a reason for hiding this comment

jscheffl left a comment

Choose a reason for hiding this comment

potiuk commented Nov 24, 2023 •

edited

Loading

jscheffl left a comment •

edited

Loading