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 missing re2 dependency to cncf.kubernetes and celery providers #33237

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

hussein-awala
Copy link
Member

re lib was replaced by re2 in Airflow core and Kubernetes (#32303), and the dependency was added only to Airflow setup (#32060). However, when we install the Kubernetes provider with an old version of Airflow, the provider will be broken because the re2 is not installed automatically, and we need to add it explicitly:

python -m venv airflow_2_4_3

airflow_2_4_3/bin/pip install apache_airflow==2.4.3

airflow_2_4_3/bin/pip install apache-airflow-providers-cncf-kubernetes==7.4.1

airflow_2_4_3/bin/python

Python 3.9.13 (main, Oct 13 2022, 21:15:33) 
[GCC 11.2.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from airflow.providers.cncf.kubernetes.pod import KubernetesPodOperator
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<PATH TO VENV>/airflow/__init__.py", line 52, in <module>
    from airflow import configuration, settings
  File "<PATH TO VENV>/airflow/configuration.py", line 41, in <module>
    import re2
ModuleNotFoundError: No module named 're2'

I think we'll have the same problem with celery provider when we want to use the new executor with the old Airflow versions (>=2.4.0, <2.7.0).

This PR add google-re2 dependency to cncf.kubernetes and celery providers.


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

@jedcunningham
Copy link
Member

Should we update the precommit check to cover providers now as well?

@potiuk
Copy link
Member

potiuk commented Aug 8, 2023

Should we update the precommit check to cover providers now as well?

Good point . I will add it as a separate PR.

@hussein-awala
Copy link
Member Author

Should we update the precommit check to cover providers now as well?

+1, but should be implemented in a separate PR as @potiuk suggested.

@potiuk
Copy link
Member

potiuk commented Aug 8, 2023

Waits for this one to be merged in #33240

@potiuk potiuk merged commit 0b528e2 into apache:main Aug 9, 2023
@potiuk
Copy link
Member

potiuk commented Aug 9, 2023

cc: @eladkal - for me sounds like a good reason to release celery and kubernetes provider soon(ish).

potiuk added a commit to potiuk/airflow that referenced this pull request Aug 9, 2023
For providers that are using google-re2 it should be declared as
dependency. Follow up after apache#33237
potiuk added a commit that referenced this pull request Aug 9, 2023
For providers that are using google-re2 it should be declared as
dependency. Follow up after #33237
@pierrejeambrun
Copy link
Member

Thanks for the fix, I missed that

@potiuk
Copy link
Member

potiuk commented Aug 9, 2023

Thanks for the fix, I missed that

We all did :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants