Re: [PR] Add UV support to venv operators [airflow]
Lee-W commented on PR #43612: URL: https://github.com/apache/airflow/pull/43612#issuecomment-2461096378 just saw it on slack. love this 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
set5think commented on PR #43612: URL: https://github.com/apache/airflow/pull/43612#issuecomment-2460737995 @jscheffl WOW THANKS FOR TAKING THIS TO THE FINISH LINE!!! It's amazing to see how simple it was to add what I think is an extremely valuable contribution!!! GOOD JOB! :heart: -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl merged PR #43612: URL: https://github.com/apache/airflow/pull/43612 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl commented on code in PR #43612:
URL: https://github.com/apache/airflow/pull/43612#discussion_r1828415014
##
providers/src/airflow/providers/standard/utils/python_virtualenv.py:
##
@@ -88,22 +145,33 @@ def prepare_virtualenv(
if pip_install_options is None:
pip_install_options = []
+if requirements is not None and requirements_file_path is not None:
+raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
+
if index_urls is not None:
_generate_pip_conf(Path(venv_directory) / "pip.conf", index_urls)
-venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
+if _use_uv():
+venv_cmd = _generate_uv_cmd(venv_directory, python_bin,
system_site_packages)
+else:
+venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
execute_in_subprocess(venv_cmd)
-if requirements is not None and requirements_file_path is not None:
-raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
-
pip_cmd = None
if requirements is not None and len(requirements) != 0:
-pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+if _use_uv():
+pip_cmd = _generate_uv_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+else:
+pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
if requirements_file_path is not None and requirements_file_path:
Review Comment:
If somebody cleans this up... pytests will fail.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on code in PR #43612:
URL: https://github.com/apache/airflow/pull/43612#discussion_r1828394305
##
providers/src/airflow/providers/standard/utils/python_virtualenv.py:
##
@@ -88,22 +145,33 @@ def prepare_virtualenv(
if pip_install_options is None:
pip_install_options = []
+if requirements is not None and requirements_file_path is not None:
+raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
+
if index_urls is not None:
_generate_pip_conf(Path(venv_directory) / "pip.conf", index_urls)
-venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
+if _use_uv():
+venv_cmd = _generate_uv_cmd(venv_directory, python_bin,
system_site_packages)
+else:
+venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
execute_in_subprocess(venv_cmd)
-if requirements is not None and requirements_file_path is not None:
-raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
-
pip_cmd = None
if requirements is not None and len(requirements) != 0:
-pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+if _use_uv():
+pip_cmd = _generate_uv_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+else:
+pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
if requirements_file_path is not None and requirements_file_path:
Review Comment:
Do you think it would be worthwhile to add comments explaining why those
conditions are set up that way? Otherwise, someone might come along later,
think they're redundant, and try to remove them?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl commented on code in PR #43612:
URL: https://github.com/apache/airflow/pull/43612#discussion_r1828387805
##
providers/src/airflow/providers/standard/utils/python_virtualenv.py:
##
@@ -88,22 +145,33 @@ def prepare_virtualenv(
if pip_install_options is None:
pip_install_options = []
+if requirements is not None and requirements_file_path is not None:
+raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
+
if index_urls is not None:
_generate_pip_conf(Path(venv_directory) / "pip.conf", index_urls)
-venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
+if _use_uv():
+venv_cmd = _generate_uv_cmd(venv_directory, python_bin,
system_site_packages)
+else:
+venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
execute_in_subprocess(venv_cmd)
-if requirements is not None and requirements_file_path is not None:
-raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
-
pip_cmd = None
if requirements is not None and len(requirements) != 0:
-pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+if _use_uv():
+pip_cmd = _generate_uv_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+else:
+pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
if requirements_file_path is not None and requirements_file_path:
Review Comment:
I also learned this while adding the feature :-D Something new every day :-D
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on code in PR #43612:
URL: https://github.com/apache/airflow/pull/43612#discussion_r1828383999
##
providers/src/airflow/providers/standard/utils/python_virtualenv.py:
##
@@ -88,22 +145,33 @@ def prepare_virtualenv(
if pip_install_options is None:
pip_install_options = []
+if requirements is not None and requirements_file_path is not None:
+raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
+
if index_urls is not None:
_generate_pip_conf(Path(venv_directory) / "pip.conf", index_urls)
-venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
+if _use_uv():
+venv_cmd = _generate_uv_cmd(venv_directory, python_bin,
system_site_packages)
+else:
+venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
execute_in_subprocess(venv_cmd)
-if requirements is not None and requirements_file_path is not None:
-raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
-
pip_cmd = None
if requirements is not None and len(requirements) != 0:
-pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+if _use_uv():
+pip_cmd = _generate_uv_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+else:
+pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
if requirements_file_path is not None and requirements_file_path:
Review Comment:
Oh got it :) , thanks jens for explaining.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on code in PR #43612:
URL: https://github.com/apache/airflow/pull/43612#discussion_r1828383999
##
providers/src/airflow/providers/standard/utils/python_virtualenv.py:
##
@@ -88,22 +145,33 @@ def prepare_virtualenv(
if pip_install_options is None:
pip_install_options = []
+if requirements is not None and requirements_file_path is not None:
+raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
+
if index_urls is not None:
_generate_pip_conf(Path(venv_directory) / "pip.conf", index_urls)
-venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
+if _use_uv():
+venv_cmd = _generate_uv_cmd(venv_directory, python_bin,
system_site_packages)
+else:
+venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
execute_in_subprocess(venv_cmd)
-if requirements is not None and requirements_file_path is not None:
-raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
-
pip_cmd = None
if requirements is not None and len(requirements) != 0:
-pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+if _use_uv():
+pip_cmd = _generate_uv_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+else:
+pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
if requirements_file_path is not None and requirements_file_path:
Review Comment:
Oh got it :) , thanks jens explaining.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on code in PR #43612:
URL: https://github.com/apache/airflow/pull/43612#discussion_r1828377367
##
providers/src/airflow/providers/standard/utils/python_virtualenv.py:
##
@@ -88,22 +145,33 @@ def prepare_virtualenv(
if pip_install_options is None:
pip_install_options = []
+if requirements is not None and requirements_file_path is not None:
+raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
+
if index_urls is not None:
_generate_pip_conf(Path(venv_directory) / "pip.conf", index_urls)
-venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
+if _use_uv():
+venv_cmd = _generate_uv_cmd(venv_directory, python_bin,
system_site_packages)
+else:
+venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
execute_in_subprocess(venv_cmd)
-if requirements is not None and requirements_file_path is not None:
-raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
-
pip_cmd = None
if requirements is not None and len(requirements) != 0:
-pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+if _use_uv():
+pip_cmd = _generate_uv_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+else:
+pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
if requirements_file_path is not None and requirements_file_path:
Review Comment:
```suggestion
else:
```
Line: 149 already checks if both requirement/requirement_file_path provided
it raise exception, so in this case here else block is fine :) ?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl commented on code in PR #43612:
URL: https://github.com/apache/airflow/pull/43612#discussion_r1828382068
##
providers/src/airflow/providers/standard/utils/python_virtualenv.py:
##
@@ -88,22 +145,33 @@ def prepare_virtualenv(
if pip_install_options is None:
pip_install_options = []
+if requirements is not None and requirements_file_path is not None:
+raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
+
if index_urls is not None:
_generate_pip_conf(Path(venv_directory) / "pip.conf", index_urls)
-venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
+if _use_uv():
+venv_cmd = _generate_uv_cmd(venv_directory, python_bin,
system_site_packages)
+else:
+venv_cmd = _generate_venv_cmd(venv_directory, python_bin,
system_site_packages)
execute_in_subprocess(venv_cmd)
-if requirements is not None and requirements_file_path is not None:
-raise ValueError("Either requirements OR requirements_file_path has to
be passed, but not both")
-
pip_cmd = None
if requirements is not None and len(requirements) != 0:
-pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+if _use_uv():
+pip_cmd = _generate_uv_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
+else:
+pip_cmd = _generate_pip_install_cmd_from_list(venv_directory,
requirements, pip_install_options)
if requirements_file_path is not None and requirements_file_path:
Review Comment:
I also scratched my head regarding this... but the logic can be... use a
venv without requirements file and without specified requirements... will
install a "plain vanilla" runtime w/o any extra packages. There are even
pytests for this. Seems to be a valid requirement having a "minimal with no
sugar" venv to execute something within.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on PR #43612: URL: https://github.com/apache/airflow/pull/43612#issuecomment-2455690460 LGTM :) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1828377689 ## providers/src/airflow/providers/standard/utils/python_virtualenv.py: ## @@ -20,15 +20,51 @@ from __future__ import annotations import os +import shutil import sys from pathlib import Path import jinja2 from jinja2 import select_autoescape +from airflow.configuration import conf from airflow.utils.process_utils import execute_in_subprocess +def _is_uv_installed() -> bool: +""" +Check if the uv tool is installed via checking if it is on the path or installed as package. Review Comment: ```suggestion Verify whether the uv tool is installed by checking if it's included in the system PATH or installed as a package. ``` -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1828346018 ## providers/src/airflow/providers/standard/provider.yaml: ## @@ -62,3 +62,20 @@ hooks: - airflow.providers.standard.hooks.filesystem - airflow.providers.standard.hooks.package_index - airflow.providers.standard.hooks.subprocess + +config: + standard: +description: Options for the standard operators Review Comment: ```suggestion description: Options for the standard provider operators. ``` -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1828302157 ## airflow/example_dags/example_branch_operator_decorator.py: ## @@ -112,7 +112,7 @@ def some_ext_py_task(): # Run the example a second time and see that it re-uses it and is faster. VENV_CACHE_PATH = tempfile.gettempdir() -@task.branch_virtualenv(requirements=["numpy~=1.24.4"], venv_cache_path=VENV_CACHE_PATH) +@task.branch_virtualenv(requirements=["numpy~=1.26.0"], venv_cache_path=VENV_CACHE_PATH) Review Comment: Separated to #43653 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1827136063 ## providers/src/airflow/providers/standard/utils/python_virtualenv.py: ## @@ -88,22 +130,33 @@ def prepare_virtualenv( if pip_install_options is None: pip_install_options = [] +if requirements is not None and requirements_file_path is not None: Review Comment: make sense :) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
potiuk commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1827059510 ## airflow/example_dags/example_branch_operator_decorator.py: ## @@ -112,7 +112,7 @@ def some_ext_py_task(): # Run the example a second time and see that it re-uses it and is faster. VENV_CACHE_PATH = tempfile.gettempdir() -@task.branch_virtualenv(requirements=["numpy~=1.24.4"], venv_cache_path=VENV_CACHE_PATH) +@task.branch_virtualenv(requirements=["numpy~=1.26.0"], venv_cache_path=VENV_CACHE_PATH) Review Comment: Yep. Separating is a good idea. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
potiuk commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1827059300 ## providers/src/airflow/providers/standard/utils/python_virtualenv.py: ## @@ -88,22 +130,33 @@ def prepare_virtualenv( if pip_install_options is None: pip_install_options = [] +if requirements is not None and requirements_file_path is not None: Review Comment: Yes @jscheffl is right - requirements migh be "empty" (as result of templating) and this would evaluate to "no requirements" (but `pip install ""` would fail) similarly requirements_file might be templated and empty result would mean "no requirements" rather than trying to do `pip install -r ""` (which would also fail).. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
potiuk commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1827059300 ## providers/src/airflow/providers/standard/utils/python_virtualenv.py: ## @@ -88,22 +130,33 @@ def prepare_virtualenv( if pip_install_options is None: pip_install_options = [] +if requirements is not None and requirements_file_path is not None: Review Comment: Yes @jscheffl is right - requirements migh be "empty" (as result of templating) and this would evaluate to "no requirements" (but `pip install -r` would fail) similarly requirements_file might be templated and empty result would mean "no requirements" rather than trying to do `pip install -r ""` (which would also fail).. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
potiuk commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1827059300 ## providers/src/airflow/providers/standard/utils/python_virtualenv.py: ## @@ -88,22 +130,33 @@ def prepare_virtualenv( if pip_install_options is None: pip_install_options = [] +if requirements is not None and requirements_file_path is not None: Review Comment: Yes @jscheffl is rightrequirements migh be "empty file" and this would evaluate to "no requirements" (but `pip install -r` would fail) similarly requirements_file might be templated and empty result would mean "no requirements" rather than trying to do `pip install -r ""` (which would also fail).. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
potiuk commented on PR #43612: URL: https://github.com/apache/airflow/pull/43612#issuecomment-2453564474 > Option "AUTO", "PIP", "UV" as Enum > AUTO=Like now, checks if UV is installed, if not uses pip Yes. It's good I think. Needs to be followed up with newsfragment explaining it. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1827039162 ## airflow/example_dags/example_branch_operator_decorator.py: ## @@ -112,7 +112,7 @@ def some_ext_py_task(): # Run the example a second time and see that it re-uses it and is faster. VENV_CACHE_PATH = tempfile.gettempdir() -@task.branch_virtualenv(requirements=["numpy~=1.24.4"], venv_cache_path=VENV_CACHE_PATH) +@task.branch_virtualenv(requirements=["numpy~=1.26.0"], venv_cache_path=VENV_CACHE_PATH) Review Comment: Note: needed to change this as else the example is broken and venv does not work with Python 3.12 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1827039162 ## airflow/example_dags/example_branch_operator_decorator.py: ## @@ -112,7 +112,7 @@ def some_ext_py_task(): # Run the example a second time and see that it re-uses it and is faster. VENV_CACHE_PATH = tempfile.gettempdir() -@task.branch_virtualenv(requirements=["numpy~=1.24.4"], venv_cache_path=VENV_CACHE_PATH) +@task.branch_virtualenv(requirements=["numpy~=1.26.0"], venv_cache_path=VENV_CACHE_PATH) Review Comment: Note: needed to change this as else the example is broken and venv does not work with Python 3.12 Might be worth a separate PR as well to fix it on Airflow 2.10 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1827038161 ## providers/src/airflow/providers/standard/utils/python_virtualenv.py: ## @@ -88,22 +130,33 @@ def prepare_virtualenv( if pip_install_options is None: pip_install_options = [] +if requirements is not None and requirements_file_path is not None: Review Comment: I _think_ there was a reason behind this and the lines just have been moved in order. There I'd propose to keep it. Even if the proposed shortage looks mot Pythonic there could be cases where Non-Null values evaluate to False. I just moved the line up to fail fast - else in previous code it was creating the venv base and then raising an exception. Not if fails before venv is started to be created. The line is originating from long long time ago from https://github.com/apache/airflow/pull/17349 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on PR #43612: URL: https://github.com/apache/airflow/pull/43612#issuecomment-2453213571 > > I'd say: > > ``` > > * add parameter to switch between uv and pip (no auto-detection - too much risk) > > ``` > > With the follow-up question(s): > > * What to default? > * Adding a config entry or forcing all DAGs do be adjusted for this param? > > Proposal: > > * Option "AUTO", "PIP", "UV" as Enum > > * AUTO=Like now, checks if UV is installed, if not uses pip > * The other options: How the user likes it > * Default is configurable and uses "AUTO" as default as provider config property > > Alternatively we can also make it like attept UV and if it fails attempt pip as fallback? I feel like default to UV is better, as now we are completely moving towards UV, so if user want to switch they can update? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
gopidesupavan commented on code in PR #43612: URL: https://github.com/apache/airflow/pull/43612#discussion_r1826863907 ## providers/src/airflow/providers/standard/utils/python_virtualenv.py: ## @@ -88,22 +130,33 @@ def prepare_virtualenv( if pip_install_options is None: pip_install_options = [] +if requirements is not None and requirements_file_path is not None: Review Comment: ```suggestion if requirements and requirements_file_path: ``` -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
jscheffl commented on PR #43612: URL: https://github.com/apache/airflow/pull/43612#issuecomment-2453204133 > I'd say: > > * add parameter to switch between uv and pip (no auto-detection - too much risk) With the follow-up question(s): * What to default? * Adding a config entry or forcing all DAGs do be adjusted for this param? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add UV support to venv operators [airflow]
potiuk commented on PR #43612: URL: https://github.com/apache/airflow/pull/43612#issuecomment-2453169990 I'd say: * add parameter to switch between uv and pip (no auto-detection - too much risk) * add unit tests :) J. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
