Fix Breeze2 autocomplete #22695
Fix Breeze2 autocomplete #22695
Conversation
|
Hey @edithturn - I finally got to the "autocomplete" on Breeze2. It turned out that "click-complete" is not needed as a dependency :). This is some old package and click has good autocomplete support out-of-the-box. I fixed |
|
Another step for |
|
wow @potiuk that is great 👏🏼 , some files are still being .sh right? |
Some files will have to remain .sh :) |
|
Cool @potiuk :) |
db8377e to
254db8e
Compare
|
I performed a few more tests and updated the autocomplete setup so that it works in all the circumstances (I hope). I refactored imports slightly so that in case indeed user does not use virtualenv with Breeze installed, click is the only requirement that is needed for the autocomplete to work. |
254db8e to
db8377e
Compare
Breeze2 autocomplete did not work because we were using some old way of adding it (via click-complete). Since then click has native (and very well working) autocomplete support without any external dependencies needed. It cannnot automatically generate the completions but it is not needed either, because we can store generated completion scripts in our repo. We also move some imports to local and catch rich_click import error to minimize dependencies needed to get autocomplete working. Setup-autocomplete install (and upgrade if needed click in case it needs to be used by autocomplete script. Fixes: apache#21164
db8377e to
393d199
Compare
|
I am very close to enable Breeze2 for everyone (@Bowrna @edithturn @eladkal ) - this one is one of the prerequiste, and I have another one coming tha reviews and de-duplicates/syncs all changes and make it "prod-ready" - Plase take a look at this one and I'd love to merge it soon. The third PR will move the old breeze to legacy and replace the current ./breeze with the new one :). I cannot wait to get the new Breeze2 in the hands of our contributors. It's sooo much nicer than the old one |
|
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. |
| @@ -102,6 +99,9 @@ def check_package_installed(package_name: str) -> bool: | |||
|
|
|||
|
|
|||
| def get_filesystem_type(filepath): | |||
| # We import it locally so that click autocomplete works | |||
| import psutil | |||
There was a problem hiding this comment.
Could you explain this part to me @potiuk? Why do we have to import locally for autocomplete to work?
There was a problem hiding this comment.
Because when autocomplete loads list of methods, it imports breeze.py and it has to import it fully to determine list of available options (quite similarly like Airflow parsing dag files to determine the structure of DAG.
Therefore we should make sure that very little number of packages need to be imported. There are two reasons:
- in the environment where autocomplete is executed, psutil might be simply not installed (And import will fail)
- importing packages takes time - we want to make autocomplete as fast as possible so that it returns list of available options immediately
This is the same thing as the one described by @blag here: #22613 (comment) and why we need to do the same in Airflow when we convert to click + click-rich.
It's also fundamentally the same case as the one described here in Airflow docs for DAG top-level code: https://airflow.apache.org/docs/apache-airflow/stable/best-practices.html?highlight=best%20practices#top-level-python-code
Breeze2 autocomplete did not work because we were using some old
way of adding it (via click-complete). Since then click has
native (and very well working) autocomplete support without any
external dependencies needed. It cannnot automatically generate
the completions but it is not needed either, because we can
store generated completion scripts in our repo.
We also move some imports to local and catch rich_click import
error to minimize dependencies needed to get autocomplete
working. Setup-autocomplete install (and upgrade if needed
click in case it needs to be used by autocomplete script.
Fixes: #21164
^ 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.