airflow icon indicating copy to clipboard operation
airflow copied to clipboard

PythonVirtualOperator fails silently when virtualenv is not installed.

Open thisiswhereitype opened this issue 1 year ago • 9 comments

Apache Airflow version

2.9.2

If "Other Airflow 2 version" selected, which one?

No response

What happened?

When using PythonVirtualOperator the venv is created by airflow as passed in the dag, in a tmp directory a standard python -m venv /tmp/.... is run. Later pip install -r ... is also run.

However, when virtualenv is not installed the failed command is not properly detected.

Later an uncaught OSError in subprocess.Popen (https://github.com/apache/airflow/blob/c310159bc2363c12110b11febd5febaab8670210/airflow/utils/process_utils.py#L184)is raised by the absent.venv/bin/pipcausing the exit logic not to fire and theTempDirectory.exit` then deletes the evidence.

See: subprocess Exceptions Additionally stderr is not logged.

What you think should happen instead?

I see in the project file there is a core-all listing including virtualenv.

Shoudn't virtualenv be a dependecy of the pip install apache-airflow? I haven't used hatch

If it shouldn't I think there should be better error handing to explain the issue.

How to reproduce

Run a PythonVirtualOperator with airflow installed in a venv that has no virtualenv.

Operating System

ws2 ubuntu 2404

Versions of Apache Airflow Providers

No response

Deployment

Virtualenv installation

Deployment details

No response

Anything else?

No response

Are you willing to submit PR?

  • [ ] Yes I am willing to submit a PR!

Code of Conduct

thisiswhereitype avatar Jun 25 '24 13:06 thisiswhereitype

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

boring-cyborg[bot] avatar Jun 25 '24 13:06 boring-cyborg[bot]

Marked it as good first issue for someone to handle.

potiuk avatar Jun 25 '24 18:06 potiuk

But we do check

https://github.com/apache/airflow/blob/5f2da71bf86f1f73acd802b66b979e77f53c8715/airflow/operators/python.py#L699-L700

eladkal avatar Jun 25 '24 18:06 eladkal

I think @thisiswhereitype to check why it did not work in his case.

potiuk avatar Jun 25 '24 18:06 potiuk

Thanks, the reason is that the is_venv_installed finds /usr/bin/virtualenv and proceeds but doesn't try to import it.

A module import would closely match what happens later.

(.venv_airflow) $ python  -c 'import importlib.util; importlib.util.find_spec("virtualenv")'
(.venv_airflow) $ which virtualenv
/usr/bin/virtualenv
(.venv_airflow) $ pip list | grep env
<no output>
(.venv_airflow) $

thisiswhereitype avatar Jun 26 '24 07:06 thisiswhereitype

Feel free to fix it, otherwise it will have to wait for someone else to pick it up.

potiuk avatar Jun 26 '24 16:06 potiuk

Hey @potiuk, could I take on this issue.

If I am correct, I should just remove the previous checks defined in the function is_venv_installed and add a new check for seeing if it venv is installed or not. Or should I preserve the previous check and just add a new check with or

STAR-173 avatar Jun 30 '24 19:06 STAR-173

What do YOU think is better?

potiuk avatar Jun 30 '24 19:06 potiuk

Removing the previous checks defined in the function is_venv_installed and add a new check for seeing if it venv is installed or not as the previous checks are not functioning as desired

STAR-173 avatar Jun 30 '24 20:06 STAR-173

Removing the previous checks defined in the function is_venv_installed and add a new check for seeing if it venv is installed or not as the previous checks are not functioning as desired

Not sure if I got it - but do what you think is best and let's discuss it.

potiuk avatar Jul 02 '24 20:07 potiuk

Hi @eladkal , Please assign this to me if @STAR-173 is not working on it.

pateash avatar Jul 15 '24 05:07 pateash

Hi @pateash , I really appreciate your concern regarding this issue but I think I will just continue to work on this. It's just been busy weeks for a bit. I will be opening a PR within this week

STAR-173 avatar Jul 15 '24 06:07 STAR-173

Looks like the PR went stale

thisiswhereitype avatar Oct 25 '24 10:10 thisiswhereitype

Looks like the PR went stale

Wopuld you like to take it over @thisiswhereitype ? Is this what the comment is about? Feel free to do it if you think it's a good idea. We have no problems with even similar people working in parallel on similar PRs - pretty much all the people contributing here are volunteers and they have day jobs, famiilies and hobbies, so they might or might not complete what they initially thought they will be able to do. Usually then someone who is eager to work on it, can take it over, and we can assign it to that person.

Shall I assign it to you @thisiswhereitype ?

potiuk avatar Oct 25 '24 18:10 potiuk

@potiuk please do, see the PR

thisiswhereitype avatar Oct 26 '24 16:10 thisiswhereitype

I commented there. You need way more to implement it.

potiuk avatar Oct 26 '24 20:10 potiuk