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

Breeze2 autocomplete requires click-complete to be installed #21164

Closed
1 of 2 tasks
potiuk opened this issue Jan 27, 2022 · 14 comments · Fixed by #22695
Closed
1 of 2 tasks

Breeze2 autocomplete requires click-complete to be installed #21164

potiuk opened this issue Jan 27, 2022 · 14 comments · Fixed by #22695
Assignees
Labels

Comments

@potiuk
Copy link
Member

potiuk commented Jan 27, 2022

Apache Airflow version

main (development)

What happened

When I setup autocomplete for Breeze2 on a "bare" system when I have no packages installed, It fails autocomplete with this error:

ModuleNotFoundError: No module named 'click_completion'
Traceback (most recent call last):
  File "/home/jarek/.pyenv/versions/3.7.9/bin/Breeze2", line 33, in <module>
    sys.exit(load_entry_point('apache-airflow-breeze', 'console_scripts', 'Breeze2')())
  File "/home/jarek/.pyenv/versions/3.7.9/bin/Breeze2", line 25, in importlib_load_entry_point
    return next(matches).load()
  File "/home/jarek/.local/lib/python3.7/site-packages/importlib_metadata/__init__.py", line 194, in load
    module = import_module(match.group('module'))
  File "/home/jarek/.pyenv/versions/3.7.9/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/jarek/code/airflow/dev/breeze/src/airflow_breeze/breeze.py", line 24, in <module>
    import click_completion
ModuleNotFoundError: No module named 'click_completion'

It seems that "autocomplete" feature of Breeze2 requires click-completion to be installed first. This is a small issue an small prerequisite but I think it is not handled by the currrent setup-autocomplete

The same happens if you install Breeze2 with pipx.

What you expected to happen

I expect that click-completion package is automatically installled in when ./Breeze2 setup-autocomplete is executed.
Also it should be described in the documentation as prerequisite

How to reproduce

  • make sure you have no packages installed in your Python "system environment" (for example: pip list | xargs pip uninstall -y )
  • type ./Breeze2

Operating System

Linux Mint 20.1.3

Versions of Apache Airflow Providers

Not relevant

Deployment

Other

Deployment details

No airflow deployment

Anything else

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@potiuk potiuk added kind:bug This is a clearly a bug area:core kind:feature Feature Requests and removed kind:bug This is a clearly a bug labels Jan 27, 2022
@edithturn
Copy link
Contributor

@potiuk please assign it to me, I am installing Linux Mint on Virtual Box

@edithturn
Copy link
Contributor

edithturn commented Jan 28, 2022

I reproduced, I am getting a similar issue with click-completion
Screenshot from 2022-01-28 13-41-01

  • I am seeing that the installation finished successfully even if this module is not installed, but then autocompletion doesn't work on Breeze

Screenshot from 2022-01-28 13-50-36

The ERROR also made reference to click-completion: ERROR: Failed building wheel for click-completion. But then:
Running setup.py install for click-completion...done

@Bowrna
Copy link
Contributor

Bowrna commented Jan 31, 2022

@potiuk I have one doubt in this. When we are using Breeze2 command, we will have the dev/breeze part pip installed in editable mode right in the user environment. In that case, if we have added click_completion in setup.cfg, wouldn't it be available in the environment?

@edithturn
Copy link
Contributor

thank you for this question @Bowrna, I will add it in setup.cfg and I will see what will happen. It should be available

@potiuk
Copy link
Member Author

potiuk commented Jan 31, 2022

@potiuk I have one doubt in this. When we are using Breeze2 command, we will have the dev/breeze part pip installed in editable mode right in the user environment. In that case, if we have added click_completion in setup.cfg, wouldn't it be available in the environment?

Not exactly. The tricky point is that you should be able to run Breeze WITHOUT activating the virtual environment. In fact this also happen when you use pipx. When you have no "click-complete" installed as "system package" then either pipx variant or the ./Breeze2 bootstrap will not work. Try it yourself:

  1. Have ./Breeze2 autocomplete installed (currently it updates the .zsh) script to activate the environment
  2. Make sure you have not activated the virtual env of Breeze
  3. Make sure you have no click-completion installed (pip uninstall click-completion) . You could have it installed by accident before, but most users who will not have it and just run pipx install or use ./Breeze2 bootstrap will not have it either.

This is what I get:

image

@potiuk
Copy link
Member Author

potiuk commented Jan 31, 2022

Basically the problem is that the user does not have the .venv from Breeze activated at the moment the TAB is pressed (it only gets activated once you run ./Breeze2 command. That's why click-completion needs to be installed as "system" package

@Bowrna
Copy link
Contributor

Bowrna commented Jan 31, 2022

Basically the problem is that the user does not have the .venv from Breeze activated at the moment the TAB is pressed (it only gets activated once you run ./Breeze2 command. That's why click-completion needs to be installed as "system" package

Got it now 👍

@Bowrna
Copy link
Contributor

Bowrna commented Feb 1, 2022

@potiuk I see that when we execute ./Breeze2, it invokes breeze.py and click_completion error is thrown. So you are suggesting to move the installation part inside setup_autocomplete to avoid this error. Can you explain to me why does ./Breeze2 code should invoke the breeze.py file? I couldn't understand that part.

airflow/Breeze2

Lines 1 to 83 in 2f4a3d4

#!/usr/bin/env python3
# isort: skip
import os
import sys
# Python <3.4 does not have pathlib
from venv import EnvBuilder
if sys.version_info.major != 3 or sys.version_info.minor < 7:
print("ERROR! Make sure you use Python 3.7+ !!")
sys.exit(1)
import subprocess
from os import execv
from pathlib import Path
if getattr(sys, 'frozen', False):
# If the application is run as a bundle, the PyInstaller bootloader
# extends the sys module by a flag frozen=True and sets the temporary app
# path into variable _MEIPASS' and sys.executable is Breeze's executable path.
AIRFLOW_SOURCES_DIR = Path(sys.executable).parent.resolve()
else:
AIRFLOW_SOURCES_DIR = Path(__file__).parent.resolve()
BUILD_DIR = AIRFLOW_SOURCES_DIR / ".build"
BUILD_BREEZE_DIR = BUILD_DIR / "breeze2"
BUILD_BREEZE_CFG_SAVED = BUILD_BREEZE_DIR / "setup.cfg.saved"
BUILD_BREEZE_VENV_DIR = BUILD_BREEZE_DIR / "venv"
BUILD_BREEZE_VENV_BIN_DIR = BUILD_BREEZE_VENV_DIR / ("Scripts" if os.name == 'nt' else "bin")
BUILD_BREEZE_VENV_PYTHON = BUILD_BREEZE_VENV_BIN_DIR / "python"
BUILD_BREEZE_VENV_BREEZE = BUILD_BREEZE_VENV_BIN_DIR / "Breeze2"
BREEZE_SOURCE_PATH = AIRFLOW_SOURCES_DIR / "dev" / "breeze"
BREEZE_SETUP_CFG_PATH = BREEZE_SOURCE_PATH / "setup.cfg"
BUILD_BREEZE_DIR.mkdir(parents=True, exist_ok=True)
def needs_installation() -> bool:
"""Returns true if Breeze's virtualenv needs (re)installation"""
if not BUILD_BREEZE_VENV_DIR.exists() or not BUILD_BREEZE_CFG_SAVED.exists():
return True
return BREEZE_SETUP_CFG_PATH.read_text() != BUILD_BREEZE_CFG_SAVED.read_text()
def save_config():
"""Saves cfg file to virtualenv to check if there is a need for reinstallation of the virtualenv"""
BUILD_BREEZE_CFG_SAVED.write_text(BREEZE_SETUP_CFG_PATH.read_text())
if needs_installation():
print(f"(Re)Installing Breeze's virtualenv in {BUILD_BREEZE_VENV_DIR}")
try:
EnvBuilder(system_site_packages=False, upgrade=True, with_pip=True, prompt="breeze").create(
str(BUILD_BREEZE_VENV_DIR)
)
except Exception as e:
# in some cases (mis-configured python) the venv creation might not work via API
# (ensurepip missing). This is the case in case of default MacOS Python and Python executable
# Bundled in Windows executable, In this case we fallback to running venv as a tool using default
# Python3 found on path (in case of Windows Bundled exe, you don't even have a current
# interpreted executable available, because Python interpreter is executed through a library.
# and sys.executable points to the Bundled exe file.
BUILD_BREEZE_VENV_DIR.mkdir(parents=True, exist_ok=True)
subprocess.run(["python3", "-m", "venv", f"{BUILD_BREEZE_VENV_DIR}"], check=True)
if os.name == 'nt':
subprocess.run(
[f"{BUILD_BREEZE_VENV_PYTHON}", "-m", "pip", "install", "--upgrade", "-e", "."],
cwd=BREEZE_SOURCE_PATH,
check=True,
)
else:
subprocess.run(
[f"{BUILD_BREEZE_VENV_PYTHON}", "-m", "pip", "install", "--upgrade", "-e", "."],
cwd=BREEZE_SOURCE_PATH,
check=True,
)
save_config()
if os.name == 'nt':
# This is the best way of running it on Windows, though it leaves the original process hanging around
subprocess.run([f"{BUILD_BREEZE_VENV_BREEZE}"] + sys.argv[1:], check=True)
else:
execv(f"{BUILD_BREEZE_VENV_BREEZE}", [f"{BUILD_BREEZE_VENV_BREEZE}"] + sys.argv[1:])

@edithturn
Copy link
Contributor

@potiuk I replicated this issue again and again, in a container, local machine, and in a Google Cloud Machine, once ./Breeze2 is installed the error will appear for the first time, then is not possible to replicate, even deleting the .build directory. To replicate it has to be in an empty environment. So I did my last test in an Instance on Google Cloud.

So to avoid this issue I suggest adding ad a pre-requirement: python3 -m pip install click-completion as a system package (correct me if I am wrong here), so the steps the user have to make will be similar to this (in a clean environment):

  • sudo apt update
  • mkdir test && cd test && git clone https://github.com/edithturn/airflow.git $$ cd airflow/
  • sudo apt install python3.8-venv
  • sudo apt install python3-pip
  • python3 -m pip install click-completion

Then ./Breeze2 will not have any issue with the installation. But as I write the lines above, it didn't show the functionality of setup-autocomplete.

Note: Even I installed click-completion as a part of the requirement, I CAN'T make a completion for Breeze2 work. (I am not sure if this could work or not, but in all the tests I did It didn't), I tested breeze setup completion and it works perfectly, but NOT BREEZE2.

To test setup-autocomplete, I run it with sudo (this is other but which Bowrna is working): sudo ./Breeze2 setup-autocomplete

In conclusion: Let me know if fixing the documentation to add these requirements could close this issue? or should I ask for help from other containers?

@potiuk
Copy link
Member Author

potiuk commented Feb 4, 2022

Yeah . I think we have too many "variables" so for now indeed fixing it in docs will be enough. One more thing we could do is - instead of "doing" stuff in the "setup-autocomplete", we could simply ask the user to make sure that "click-complete" is installed as system package.

Writing this as [yellow] warning and some example instructions (same as in docs) as the setup-autocomplete output should do the job.

@Bowrna
Copy link
Contributor

Bowrna commented Feb 5, 2022

@potiuk I see that when we execute ./Breeze2, it invokes breeze.py and click_completion error is thrown. So you are suggesting to move the installation part inside setup_autocomplete to avoid this error. Can you explain to me why does ./Breeze2 code should invoke the breeze.py file? I couldn't understand that part.

airflow/Breeze2

Lines 1 to 83 in 2f4a3d4

#!/usr/bin/env python3
# isort: skip
import os
import sys
# Python <3.4 does not have pathlib
from venv import EnvBuilder
if sys.version_info.major != 3 or sys.version_info.minor < 7:
print("ERROR! Make sure you use Python 3.7+ !!")
sys.exit(1)
import subprocess
from os import execv
from pathlib import Path
if getattr(sys, 'frozen', False):
# If the application is run as a bundle, the PyInstaller bootloader
# extends the sys module by a flag frozen=True and sets the temporary app
# path into variable _MEIPASS' and sys.executable is Breeze's executable path.
AIRFLOW_SOURCES_DIR = Path(sys.executable).parent.resolve()
else:
AIRFLOW_SOURCES_DIR = Path(__file__).parent.resolve()
BUILD_DIR = AIRFLOW_SOURCES_DIR / ".build"
BUILD_BREEZE_DIR = BUILD_DIR / "breeze2"
BUILD_BREEZE_CFG_SAVED = BUILD_BREEZE_DIR / "setup.cfg.saved"
BUILD_BREEZE_VENV_DIR = BUILD_BREEZE_DIR / "venv"
BUILD_BREEZE_VENV_BIN_DIR = BUILD_BREEZE_VENV_DIR / ("Scripts" if os.name == 'nt' else "bin")
BUILD_BREEZE_VENV_PYTHON = BUILD_BREEZE_VENV_BIN_DIR / "python"
BUILD_BREEZE_VENV_BREEZE = BUILD_BREEZE_VENV_BIN_DIR / "Breeze2"
BREEZE_SOURCE_PATH = AIRFLOW_SOURCES_DIR / "dev" / "breeze"
BREEZE_SETUP_CFG_PATH = BREEZE_SOURCE_PATH / "setup.cfg"
BUILD_BREEZE_DIR.mkdir(parents=True, exist_ok=True)
def needs_installation() -> bool:
"""Returns true if Breeze's virtualenv needs (re)installation"""
if not BUILD_BREEZE_VENV_DIR.exists() or not BUILD_BREEZE_CFG_SAVED.exists():
return True
return BREEZE_SETUP_CFG_PATH.read_text() != BUILD_BREEZE_CFG_SAVED.read_text()
def save_config():
"""Saves cfg file to virtualenv to check if there is a need for reinstallation of the virtualenv"""
BUILD_BREEZE_CFG_SAVED.write_text(BREEZE_SETUP_CFG_PATH.read_text())
if needs_installation():
print(f"(Re)Installing Breeze's virtualenv in {BUILD_BREEZE_VENV_DIR}")
try:
EnvBuilder(system_site_packages=False, upgrade=True, with_pip=True, prompt="breeze").create(
str(BUILD_BREEZE_VENV_DIR)
)
except Exception as e:
# in some cases (mis-configured python) the venv creation might not work via API
# (ensurepip missing). This is the case in case of default MacOS Python and Python executable
# Bundled in Windows executable, In this case we fallback to running venv as a tool using default
# Python3 found on path (in case of Windows Bundled exe, you don't even have a current
# interpreted executable available, because Python interpreter is executed through a library.
# and sys.executable points to the Bundled exe file.
BUILD_BREEZE_VENV_DIR.mkdir(parents=True, exist_ok=True)
subprocess.run(["python3", "-m", "venv", f"{BUILD_BREEZE_VENV_DIR}"], check=True)
if os.name == 'nt':
subprocess.run(
[f"{BUILD_BREEZE_VENV_PYTHON}", "-m", "pip", "install", "--upgrade", "-e", "."],
cwd=BREEZE_SOURCE_PATH,
check=True,
)
else:
subprocess.run(
[f"{BUILD_BREEZE_VENV_PYTHON}", "-m", "pip", "install", "--upgrade", "-e", "."],
cwd=BREEZE_SOURCE_PATH,
check=True,
)
save_config()
if os.name == 'nt':
# This is the best way of running it on Windows, though it leaves the original process hanging around
subprocess.run([f"{BUILD_BREEZE_VENV_BREEZE}"] + sys.argv[1:], check=True)
else:
execv(f"{BUILD_BREEZE_VENV_BREEZE}", [f"{BUILD_BREEZE_VENV_BREEZE}"] + sys.argv[1:])

@potiuk could you share your view on this?

@edithturn
Copy link
Contributor

@potiuk what are you suggesting here, it is to avoid touching Breeze2, and instead, add in airflow/docs something like apache-airflow-breeze2 and write some warning in yellow about the installation of click-complete as a system package.

@potiuk
Copy link
Member Author

potiuk commented Feb 7, 2022

@potiuk what are you suggesting here, it is to avoid touching Breeze2, and instead, add in airflow/docs something like apache-airflow-breeze2 and write some warning in yellow about the installation of click-complete as a system package.

Yep. If it is not installed when you run Breeze2.

BTW. I might actually take a stab on it myself BTW. I had a discussion with Bowrna that made me think that maybe there is another problem. So do not bother about this one for a while.

This one might be a little "tricky" :)

@potiuk potiuk self-assigned this Feb 7, 2022
@edithturn
Copy link
Contributor

@potiuk I see, please if you need a test let me know, I can test it faster too, I have free credits on Google Cloud it could help.

potiuk added a commit to potiuk/airflow that referenced this issue Apr 3, 2022
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
potiuk added a commit that referenced this issue Apr 3, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants