You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/04/04 21:43:50 UTC
[GitHub] [airflow] potiuk opened a new pull request, #22740: Switch to `pipx` as the only installation Breeze2 method
potiuk opened a new pull request, #22740:
URL: https://github.com/apache/airflow/pull/22740
Switching Breeze2 to only use `pipx` for installation of Breeze2
due to problems it might cause for autocompletion if entrypoint
is not avaiable on PATH.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843590953
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -234,7 +235,7 @@
{
"name": "Setup autocomplete flags",
"options": [
- "--force-setup",
+ "--force§",
Review Comment:
Accidental?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090558881
Yep. Added and improved "Breeze2" bash script (will be swapped with the old ./breeze eventually temporarily )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843648257
##########
scripts/ci/pre_commit/pre_commit_update_breeze_setup_hash.py:
##########
@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import hashlib
+from pathlib import Path
+
+if __name__ not in ("__main__", "__mp_main__"):
+ raise SystemExit(
+ "This file is intended to be executed as an executable program. You cannot use it as a module."
+ f"To execute this script, run ./{__file__} [FILE] ..."
+ )
+
+AIRFLOW_SOURCES_ROOT = Path(__file__).parent.parent.parent.parent
+BREEZE_SOURCES_ROOT = AIRFLOW_SOURCES_ROOT / "dev" / "breeze"
+
+
+def get_package_setup_metadata_hash() -> str:
+ """
+ Retrieves hash of setup.py and setup.cfg files.
+
+ This is used in order to determine if we need to upgrade Breeze, because some
+ setup files changed. Blake2b algorithm will not be flagged by security checkers
+ as insecure algorithm (in Python 3.9 and above we can use `usedforsecurity=False`
+ to disable it, but for now it's better to use more secure algorithms.
+ """
+ the_hash = hashlib.new("blake2b")
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.py").read_bytes())
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.cfg").read_bytes())
Review Comment:
Yeah. We don't have anything substantial in it yet - but I believe setuptools/pip are not yet ready to switch to pyproject.toml . Do you think it's worth to move more stuff now ?
what should we move there from cfg ? Or maybe we could already get rid of setup.py or setup.cfg ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090855290
> Thanks for the further details, and the logic is sound. I suppose I'm someone who likes Bash as well so my opinion is biased 😁
Yeah. Bash was cool. Until it was not :).
The main reason why it was cool was that it "just worked". That's the only real benefit of bash. Once this is gone - there are only drawbacks left.
Dealing with MacOS version of Bash taught me that it's not cool any more. I think for all practical purposes of anything more than 20-50 lines of scripts without any real logic - it's a RIP for me. I'm done :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843592909
##########
scripts/ci/pre_commit/pre_commit_update_breeze_setup_hash.py:
##########
@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import hashlib
+from pathlib import Path
+
+if __name__ not in ("__main__", "__mp_main__"):
+ raise SystemExit(
+ "This file is intended to be executed as an executable program. You cannot use it as a module."
+ f"To execute this script, run ./{__file__} [FILE] ..."
+ )
+
+AIRFLOW_SOURCES_ROOT = Path(__file__).parent.parent.parent.parent
+BREEZE_SOURCES_ROOT = AIRFLOW_SOURCES_ROOT / "dev" / "breeze"
+
+
+def get_package_setup_metadata_hash() -> str:
+ """
+ Retrieves hash of setup.py and setup.cfg files.
+
+ This is used in order to determine if we need to upgrade Breeze, because some
+ setup files changed. Blake2b algorithm will not be flagged by security checkers
+ as insecure algorithm (in Python 3.9 and above we can use `usedforsecurity=False`
+ to disable it, but for now it's better to use more secure algorithms.
+ """
+ the_hash = hashlib.new("blake2b")
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.py").read_bytes())
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.cfg").read_bytes())
Review Comment:
We should add `pyproject.toml` as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843747505
##########
scripts/ci/pre_commit/pre_commit_update_breeze_setup_hash.py:
##########
@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import hashlib
+from pathlib import Path
+
+if __name__ not in ("__main__", "__mp_main__"):
+ raise SystemExit(
+ "This file is intended to be executed as an executable program. You cannot use it as a module."
+ f"To execute this script, run ./{__file__} [FILE] ..."
+ )
+
+AIRFLOW_SOURCES_ROOT = Path(__file__).parent.parent.parent.parent
+BREEZE_SOURCES_ROOT = AIRFLOW_SOURCES_ROOT / "dev" / "breeze"
+
+
+def get_package_setup_metadata_hash() -> str:
+ """
+ Retrieves hash of setup.py and setup.cfg files.
+
+ This is used in order to determine if we need to upgrade Breeze, because some
+ setup files changed. Blake2b algorithm will not be flagged by security checkers
+ as insecure algorithm (in Python 3.9 and above we can use `usedforsecurity=False`
+ to disable it, but for now it's better to use more secure algorithms.
+ """
+ the_hash = hashlib.new("blake2b")
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.py").read_bytes())
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.cfg").read_bytes())
Review Comment:
I plan to experiment on converting things to pyproject.toml ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090137370
Actually I added even more - now you do not have to even run the `reinstall` manually. If you will answer yes, Breeze will re-install itself from the right place automaticall.
@uranusjr @o-nikolas - following our earlier dscussions - I think it strikes the right balance between the "standard" way of managing packages with pipx. not having to create a separate virtualenv but also making sure that user will not be surprised by some development changes by switching to a different source tree.
cc: @Bowrna @edithturn
Right now we have those things in place:
1) Everything works smoothly (and from PATH) of the user when no setup/pyproject changed (latest sources will be used automatically via --editable flag).
2) We have now `selfupgrade` command in Breeze (it runs the right `pipx install` command behind the scenes)
3) If user installls airflow in non-editable mode, it will refuse to start (unless running help of selfupgrade)
4) If there are changes to the setup/pyproject files user will be warned and instructed how to reinstall and will even be offerent to reinstall breeze by pressing y within 3 seconds (it will proceed with the installed version if not answered)
5) In cae we run Breeze2 from different sources of Airflow we will also get warning - explaining that Breeze is installed from elsewhere and providing instructions and offering the user to reinstall Breeze from the current sources
Also all those are not checked and warning is not printed if:
* Breeze2 is run asautocomplete
* Breeze2 is run with `--help` command
* Breeze2 is run with `selfupgrade` command already
That allows to get autocomplete, get help on commands without extra questions/warnings and selfupgrade airflow without recurrence :).
I think that covers all the cases I can think of :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843673028
##########
scripts/ci/pre_commit/pre_commit_update_breeze_setup_hash.py:
##########
@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import hashlib
+from pathlib import Path
+
+if __name__ not in ("__main__", "__mp_main__"):
+ raise SystemExit(
+ "This file is intended to be executed as an executable program. You cannot use it as a module."
+ f"To execute this script, run ./{__file__} [FILE] ..."
+ )
+
+AIRFLOW_SOURCES_ROOT = Path(__file__).parent.parent.parent.parent
+BREEZE_SOURCES_ROOT = AIRFLOW_SOURCES_ROOT / "dev" / "breeze"
+
+
+def get_package_setup_metadata_hash() -> str:
+ """
+ Retrieves hash of setup.py and setup.cfg files.
+
+ This is used in order to determine if we need to upgrade Breeze, because some
+ setup files changed. Blake2b algorithm will not be flagged by security checkers
+ as insecure algorithm (in Python 3.9 and above we can use `usedforsecurity=False`
+ to disable it, but for now it's better to use more secure algorithms.
+ """
+ the_hash = hashlib.new("blake2b")
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.py").read_bytes())
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.cfg").read_bytes())
Review Comment:
Added it and also handled edge-case when Breeze is run on old version of sources (where ./dev/breeze/ is missin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090813081
> Let me elaborate on that :).
Thanks for the further details, and the logic is sound. I suppose I'm someone who likes Bash as well so my opinion is biased :grin:
I could see a solution where users build the required venvs from using a combination of scripts and documentation provided in the Airflow repo, but at that point maybe we're just implementing a more tightly scoped version of pipx for the sake of "simplifying" things.
Thanks for all the work here Jarek :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1088052284
Hey @uranusjr - it's about the time we switch to the new Breeze and after trying various behaviours, I think the idea of only using `pipx` for installation that you advoceated for is the best (with a few caveats).
I described the caveats in ADR 10 (see the PR) with some description of what I think is needed in order to get `pipx-only' installation works for us. I implemented in the PR all the features but I need your help in the last one.
The features I implemented:
1) fail if Breeze is installed in non-editable mode. This was easy and needed, this way we prevent users from mistakenly install breeze without editable mode and will make sure they are not using outdated pipx if we the sources change on checkout
2) warn (and add small delay) if we detect if Breeze runs in another "airflow" source tree - this is needed because the version/deps of Breeze in that source tree might be different and we might need the different version in the source tree. But this is just a warning and we allow Breeze to work in the other source tree as **most likely** it will work - and it will nicely handle the case when user has mutliple airflow source dirs without having to reinstall breeze
Now - I need your help in the third case::
3) I need to find out if the setup.py/cfg changed since the last installation and also warn the user. Not having some deps installed in the future might be source of confusion with cryptic errors, so I would like to detect the situation and continue runnign but warn the user (with little timeout). I almost have it - the only thing I need is to save the setup.py/cfg files in a cache **right** after installation. Since we are always running in "editable" mode, there used to be a way to run "develop" after installation. I think though it has changed wiht newer `pip` versiosn (on the road to pyproject.toml). Is there a way to achieve it in a "portable" and PEP 660 - compliant way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090012061
All addressed :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090560230
And it's Python, Python everywhere :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1089237165
Hey @uranusjr (and others) - I think I found a very good way to add the warning. In short:
1) I update the README.md of `apache-airlfow-breeze` with pre-commit to contain hash of setup.py + setup.cfg
2) This README.md is used during installation so README.md with the hash is part of the installed package
3) When I start Breeze, I verify the hash of setup.py/cfg from sources with the one stored in the readme of installed package (and print warning to upgrade if they are different)
This is - I think - the best way of handling it. It has mutliple advantages:
* pre-commit updates the hash when there is a change in setup files - so whenever you install Breeze with `pipx install -e ./dev/breeze' you should get the "setup hash consistent with the instalallation time" in the package
* Breeeze2 is always installed in --editable mode (we fail Breeze if it is not) and this means that we always can recalculate hashes of current setup.py/cfg and warn if they are different
* there are no changes to setup scripts. in fact, even when we convert to pyproject.toml, the only change will be to check the hash of pyproject.toml. The nice thing is that there is no dynamic code execution at setup time - everything is static
I cannot see disadvantages :) . The users gets nice feedback and instructions and slightly annoying delay when they should reinstall Breeze which I think is the best approach.
Looking forward to merging that one - this is the one-but-last to switch to the new Breeze2 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843648257
##########
scripts/ci/pre_commit/pre_commit_update_breeze_setup_hash.py:
##########
@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import hashlib
+from pathlib import Path
+
+if __name__ not in ("__main__", "__mp_main__"):
+ raise SystemExit(
+ "This file is intended to be executed as an executable program. You cannot use it as a module."
+ f"To execute this script, run ./{__file__} [FILE] ..."
+ )
+
+AIRFLOW_SOURCES_ROOT = Path(__file__).parent.parent.parent.parent
+BREEZE_SOURCES_ROOT = AIRFLOW_SOURCES_ROOT / "dev" / "breeze"
+
+
+def get_package_setup_metadata_hash() -> str:
+ """
+ Retrieves hash of setup.py and setup.cfg files.
+
+ This is used in order to determine if we need to upgrade Breeze, because some
+ setup files changed. Blake2b algorithm will not be flagged by security checkers
+ as insecure algorithm (in Python 3.9 and above we can use `usedforsecurity=False`
+ to disable it, but for now it's better to use more secure algorithms.
+ """
+ the_hash = hashlib.new("blake2b")
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.py").read_bytes())
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.cfg").read_bytes())
Review Comment:
Yeah. We don't have it yet - but I believe setuptools/pip are not yet ready to switch to pyproject.toml . Do you think it's worth to add it now ? what should we move there from cfg ? Or maybe we could already get rid of setup.py or setup.cfg ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843643609
##########
dev/breeze/src/airflow_breeze/breeze.py:
##########
@@ -234,7 +235,7 @@
{
"name": "Setup autocomplete flags",
"options": [
- "--force-setup",
+ "--force§",
Review Comment:
I unified it force as `pipx install uses --force as well - but yeah the § was accidental :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090551273
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1088053260
cc: @Bowrna @edithturn - > one more thing before the prime time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090641375
> I'll have to take a while to read in detail, but at first blush this looks like a tonne of machinery and tooling required just to make pipx work for this development application. I still worry about maintaining it all, future edge cases, and the cognitive overload for the user to understand what exactly is going on here. I suppose that I still just don't see what this approach is buying us that is worth all this effort/code, over just using a local executable/script. But as always I'm happy to commit and help maintain it if the rest of the community thinks we're getting a big win from using this approach :)
Let me elaborate on that :).
I've been dealing wiht this several times in the past at various incarnations, and what we get here is really the "best" approach so far.
The root problem is not really a problem of `pipx`. It is changing from Bash to Python that creates this cognitive overload and the need for machinery.
The "NICE" part of Bash is that it supposedly "just works" when you use just bash + POSIX tools. It's not entirely true any more actually. Apple with MacOS sticking to old version (due to licensing issues), some POSIXY and old tooling on Mac, lack of support for Windows without WSL2 make it "almost just works". And it's only me who "likes" bash from the community.
Choosing Python is good idea for Airlfow. It has good reasoning because all airflow contributors know Python. But it has a caveat - you need to maintain virtual environment for anything but simplest scripts. There are optional dependencies, that need to be installed, in the right versions, they change over time, new are - inevitably - added as you need them. And there are no easy ways around that. Even now, those are the current dependencies for Breeze:
```
click
inputimeout
importlib-metadata>=4.4; python_version < "3.8"
pendulum
psutil
pytest
pytest-xdist
pyyaml
requests
rich
rich_click
```
And it is mind-boggling on its own that you have to maintain a separate small "virtualenv" to install a tool to manage development environment for Breeze which is also Python based and has its own dependencies (but in docker container). But you do, in fact. It's inevitable. Been there, done that few times
THIS is the problem, not the `pipx`.
The `pipx` solution actually makes it easier - because it "hides" the small venv and leaves you with the read-to-use almost-binary entrypoint that you can use. But it does not solve the upgrade/maintenance - it assumes that you have installable tool and that you manually manage it when it changes , but ... we really want to make sure that we dynamically manage it when we add new stuff, dependencies. The problem with Python deps is that when they are not installed - you find out that by (as Breeze user) by seing a cryptic ImportError stack trace. Hardly friendly message.
And what happens next are the developers (who only really care about developing Airflow code and do not know anything about the tool that manges their dev env) will complain that their tool stopped working and post the import error stack traces they got.
We basically "killed" the idea of "script that just works" when we decided to switch to Python. Actually `pipx` makes it easier to manage the env not more difficult :).
Ideally (and this was my initial idea) those kind of tools might be written in 'golang'. And this is for example what `astro-cli` does https://github.com/astronomer/astro-cli to manage "DAG" development environment. It compiles to statically linked per-platform binary and adding new dependencies just cause bigger binary.
But the big disadvantage of golang is that you need to build and distribute and upgrade the binary and that ... our community does not use/know golang. So Python is a better choice for us - even if we need more "machinery" to keep it updated.
The `pipx` solution (with the machinery) gets as close as it gets to it. You simply "self-upgrade" and you have a platform-independent 'breeze" binary on the path - wiht all dependencies updated to the ones you need at this very version. Without worrying about it, without posting "I have that stack trace", "it does not work".
Did you realise that you need pyaml` and `rich` and `inputimeout` installed to run the new Breeze up-and-running?
You should not even know that - honestly. What you need as a dependency for your tool is an internal detail. And this is what we get with this approach. You do not not know what you need. And when tomorrow you will need `requests` library for whatever reason - you will not know it either. You will jus get information that you should update and an offer to do it for you automatically if you answer "yes".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #22740:
URL: https://github.com/apache/airflow/pull/22740
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843590548
##########
dev/breeze/src/airflow_breeze/__init__.py:
##########
@@ -14,3 +14,5 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+NAME = "Breeze2"
+VERSION = "0.0.1"
Review Comment:
When do we need to bump this? Maybe we should just set this to `2`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090322335
BTW. Changed `selfupgrade` to `self-upgrade` for consistency
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r844208707
##########
scripts/ci/pre_commit/pre_commit_update_breeze_setup_hash.py:
##########
@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import hashlib
+from pathlib import Path
+
+if __name__ not in ("__main__", "__mp_main__"):
+ raise SystemExit(
+ "This file is intended to be executed as an executable program. You cannot use it as a module."
+ f"To execute this script, run ./{__file__} [FILE] ..."
+ )
+
+AIRFLOW_SOURCES_ROOT = Path(__file__).parent.parent.parent.parent
+BREEZE_SOURCES_ROOT = AIRFLOW_SOURCES_ROOT / "dev" / "breeze"
+
+
+def get_package_setup_metadata_hash() -> str:
+ """
+ Retrieves hash of setup.py and setup.cfg files.
+
+ This is used in order to determine if we need to upgrade Breeze, because some
+ setup files changed. Blake2b algorithm will not be flagged by security checkers
+ as insecure algorithm (in Python 3.9 and above we can use `usedforsecurity=False`
+ to disable it, but for now it's better to use more secure algorithms.
+ """
+ the_hash = hashlib.new("blake2b")
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.py").read_bytes())
+ the_hash.update((BREEZE_SOURCES_ROOT / "setup.cfg").read_bytes())
Review Comment:
Cool! Happy to review and learn :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22740:
URL: https://github.com/apache/airflow/pull/22740#discussion_r843641365
##########
dev/breeze/src/airflow_breeze/__init__.py:
##########
@@ -14,3 +14,5 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+NAME = "Breeze2"
+VERSION = "0.0.1"
Review Comment:
Yeah. Actually we do not have version yet. I have another PR when I switch it to 2.0.0 indeed (when I switch everyone to use the new Breeze
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090559863
Very impressive work @potiuk!
> Right now we have those things in place:
I'll have to take a while to read in detail, but at first blush this looks like a tonne of machinery and tooling required just to make pipx work for this development application. I still worry about maintaining it all, future edge cases, and the cognitive overload for the user to understand what exactly is going on here. I suppose that I still just don't see what this approach is buying us that is worth all this effort/code, over just using a local executable/script.
But as always I'm happy to commit and help maintain it if the rest of the community thinks we're getting a big win from using this approach :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22740: Switch to `pipx` as the only installation Breeze2 method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22740:
URL: https://github.com/apache/airflow/pull/22740#issuecomment-1090545981
BTW. I am super-happy with the state of it now.
I tested it quite a bit and i think we have **all** the problems solved that I saw with the `pipx` solution - for dev tools like breeze is which is installed from sources. I have no more unfulfilled requirements basically.
With the automated self-upgrade capabilities (thanks to `pipx` handling all the heavy lifting for installations and upgrades behind the scenes) we will have nice self-maintaining eventually-consistent-for-every-developer tool that can evolve together with Airflow code (and we are using rather standard-compliant ways of managing the mini-python project with its dependencies that is future-proof as well - and we can make it pyproject.toml only in the future I guess).
That was a good idea to push for `pipx` @uranusjr - I am glad you were persistent :), enough for me to make the effort to try it more and make it actually works well in all the development scenarios.
Once this is merged, I am planning to make last, final PR with documentation update and swapping out old breeze with new one 🤞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org