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.

Sign up for GitHub

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

Jump to bottom

Add UV support to venv operators #43612

Merged
merged 9 commits into from
Nov 4, 2024
Merged

Add UV support to venv operators #43612

merged 9 commits into from
Nov 4, 2024

Conversation

jscheffl
Copy link
Contributor

@jscheffl jscheffl commented Nov 2, 2024

Follow-up on #43553 and #43568

Adds support for UV for the VirtualEnv operators.

Don't know if it is acceptable like this, it is auto-detecting: if UV is installed then it is using UV.
Do we need to make this configurable or is an auto-detection sufficient?

@potiuk
Copy link
Member

potiuk commented Nov 2, 2024 •
edited
Loading

I'd say:

  • add parameter to switch between uv and pip (no auto-detection - too much risk)
  • add unit tests :)

@jscheffl
Copy link
Contributor Author

jscheffl commented Nov 2, 2024 •
edited
Loading

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?

gopidesupavan
gopidesupavan reviewed Nov 2, 2024
View reviewed changes
Copy link
Member

@gopidesupavan gopidesupavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small change :)

providers/src/airflow/providers/standard/utils/python_virtualenv.py Show resolved Hide resolved
@gopidesupavan
Copy link
Member

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?

@jscheffl jscheffl force-pushed the feature/add-uv-support-to-venv-operator branch from f083716 to 212cfb6 Compare November 3, 2024 18:29
@potiuk
Copy link
Member

potiuk commented Nov 3, 2024

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.

@jscheffl jscheffl force-pushed the feature/add-uv-support-to-venv-operator branch from 212cfb6 to 4f439ce Compare November 4, 2024 19:51
@jscheffl @gopidesupavan
Update providers/src/airflow/providers/standard/provider.yaml
302905d 
Co-authored-by: GPK <gopidesupavan@gmail.com>
@jscheffl @gopidesupavan
Update providers/src/airflow/providers/standard/utils/python_virtuale…
3c8c3aa 
…nv.py

Co-authored-by: GPK <gopidesupavan@gmail.com>
@gopidesupavan
Copy link
Member

LGTM :)

@jscheffl jscheffl merged commit 286075f into apache:main Nov 4, 2024
56 checks passed
@gopidesupavan gopidesupavan mentioned this pull request Nov 5, 2024
Merged
@set5think
Copy link
Contributor

@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! ❤️

@Lee-W
Copy link
Member

Lee-W commented Nov 7, 2024

just saw it on slack. love this one!

ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
@jscheffl @gopidesupavan @ellisms
Add UV support to venv operators (apache#43612)
19d75c9 
* Add UV support to venv operators

* Uups, allow creation also when requirements+pip.conf are used

* Fix venv numpy example which needs to be 1.26 at least to be working in Python 3.12

* Review feedback and pytests

* Fix pytests

* Revert fix in examples

* Add newsfragment

* Update providers/src/airflow/providers/standard/provider.yaml

Co-authored-by: GPK <gopidesupavan@gmail.com>

* Update providers/src/airflow/providers/standard/utils/python_virtualenv.py

Co-authored-by: GPK <gopidesupavan@gmail.com>

---------

Co-authored-by: GPK <gopidesupavan@gmail.com>
@Lee-W Lee-W mentioned this pull request Nov 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gopidesupavan gopidesupavan gopidesupavan approved these changes

@potiuk potiuk Awaiting requested review from potiuk

Assignees
No one assigned
Labels
area:providers provider:standard
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jscheffl @potiuk @gopidesupavan @set5think @Lee-W