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

Add ‘—python $(which python)’ to ‘pipx -e ./dev/breeze’ command to avoid error regarding python version compatibility #35620

Conversation

sseung00921
Copy link
Contributor

@sseung00921 sseung00921 commented Nov 14, 2023

I requested the same PR about 1 hour ago but I closed the PR by my mistake. I am sorry. you can delete the closed PR. The PR is same as this.

During setting Breeze environment according to guide of CONTRIBUTORS_QUICK_START.rst(https://github.com/apache/airflow/blob/main/CONTRIBUTORS_QUICK_START.rst),
I got error below at installing Breeze with pipx part.
image
My airflow-env(virtual python env)'s python version is 3.8.5 as suggested in above guide doc.
And My local machine's global python version is 3.12.0.
Both version is installed and set by pyenv and my OS is Mac/Intel

I don't know the exact principle of pipx but I think that it uses its own python version when Installing something(I am not sure). So I solve this error by adding '--python $(which python)' option so that I can make pipx use the python version of my airflow-env.

I am not sure others will meet the same error,. If my PR is not appropriate I will be thankful for any feedback


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

@sseung00921 sseung00921 force-pushed the Add-python-option-to-pipx-command-for-installing-breeze branch from 9c15238 to bc021ce Compare November 14, 2023 04:47
@sseung00921 sseung00921 force-pushed the Add-python-option-to-pipx-command-for-installing-breeze branch from bc021ce to 4d02e85 Compare November 15, 2023 00:01
@potiuk
Copy link
Member

potiuk commented Nov 15, 2023

Generally yes. pipx will use the global python. This is expected because pipx creates it's own virtualenv and uses the python version you specified it to use.

I think better suggestion for it will be to add few suggestions for users to follow. I think defaulting to which python is generally bad idea (for example it will not run if your python 3.12 is global and you are NOT in a virtual env..

So I'd suggest to describe the reaon and explaining few options:

a) change global to not use python 3.12
b) specifying python version (for example --python python3.10)
c) using which python ifyou want to use python that you have on a path in the current virtualenv

Also this problem will likely go away soon, I am just about to remove the limit for Python 3.12 and change the installation of breeze to use pyproject.toml and different build tools so it might be not needed anyway.

@sseung00921
Copy link
Contributor Author

@potiuk Thank you for your comment!! I agree with your opinion. Now is it Ok for me to close this PR or should I remain it open??

@potiuk
Copy link
Member

potiuk commented Nov 15, 2023

Proper fix here: #35652

@potiuk
Copy link
Member

potiuk commented Nov 15, 2023

Closing in favour of #35652 which migrates breeze to PEP-517 compliant way of installation and allows to remove Python 3.12 limitation for it.

@potiuk potiuk closed this Nov 15, 2023
@potiuk
Copy link
Member

potiuk commented Nov 21, 2023

It turned out that there are still cases when PEP-517 did not help - so I am resurrecting that one

potiuk added a commit to potiuk/airflow that referenced this pull request Nov 21, 2023
For some reason (likely importing some stuff from setup.py in the
old days) pendulum was added as dependency in breeze - which still
caused a problem when `pipx` decided to use Python 3.12 to install
Breeze (despite apache#35652 that was supposed to supersede apache#35620).

The apache#35620 adding a need to specify python additionally when you
install breeze added it's own complexity (which python?), it
turns out that breeze does not need to have pendulum installed
at all now (we stopped depending on airflow being installed
and stopped importing things from setup.py or __version__ in
favour of directly parsing __version__ variable from python code.

This PR removes pendulum entirely as Breeze dependency.
@potiuk
Copy link
Member

potiuk commented Nov 21, 2023

Right - I simply removed pendulum as dependency in Breeze. I believe it's not needed for anything there (we used it in the past to mitigate cases where we imported something from airflow) - but I don't think it's actually used now

#35786

potiuk added a commit that referenced this pull request Nov 21, 2023
For some reason (likely importing some stuff from setup.py in the
old days) pendulum was added as dependency in breeze - which still
caused a problem when `pipx` decided to use Python 3.12 to install
Breeze (despite #35652 that was supposed to supersede #35620).

The #35620 adding a need to specify python additionally when you
install breeze added it's own complexity (which python?), it
turns out that breeze does not need to have pendulum installed
at all now (we stopped depending on airflow being installed
and stopped importing things from setup.py or __version__ in
favour of directly parsing __version__ variable from python code.

This PR removes pendulum entirely as Breeze dependency.
ephraimbuddy pushed a commit that referenced this pull request Nov 23, 2023
For some reason (likely importing some stuff from setup.py in the
old days) pendulum was added as dependency in breeze - which still
caused a problem when `pipx` decided to use Python 3.12 to install
Breeze (despite #35652 that was supposed to supersede #35620).

The #35620 adding a need to specify python additionally when you
install breeze added it's own complexity (which python?), it
turns out that breeze does not need to have pendulum installed
at all now (we stopped depending on airflow being installed
and stopped importing things from setup.py or __version__ in
favour of directly parsing __version__ variable from python code.

This PR removes pendulum entirely as Breeze dependency.
ephraimbuddy pushed a commit that referenced this pull request Nov 26, 2023
For some reason (likely importing some stuff from setup.py in the
old days) pendulum was added as dependency in breeze - which still
caused a problem when `pipx` decided to use Python 3.12 to install
Breeze (despite #35652 that was supposed to supersede #35620).

The #35620 adding a need to specify python additionally when you
install breeze added it's own complexity (which python?), it
turns out that breeze does not need to have pendulum installed
at all now (we stopped depending on airflow being installed
and stopped importing things from setup.py or __version__ in
favour of directly parsing __version__ variable from python code.

This PR removes pendulum entirely as Breeze dependency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants