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

Dont add extra deps for providers with same cross provider deps #44341

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

amoghrajesh
Copy link
Contributor

closes: #44195

After changes:

deps is ['apache-airflow-providers-common-sql']
extras is  {'amazon': ['apache-airflow-providers-amazon'], 'common.sql': ['apache-airflow-providers-common-sql'], 'openlineage': ['apache-airflow-providers-openlineage']}
EXTRAS ARE {'amazon': ['apache-airflow-providers-amazon>=2.6.0'], 'openlineage': ['apache-airflow-providers-openlineage']}


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

@amoghrajesh
Copy link
Contributor Author

amoghrajesh commented Nov 26, 2024

@potiuk @eladkal @gopidesupavan can you take a look when you have some time?

@amoghrajesh
Copy link
Contributor Author

@potiuk @gopidesupavan i handled it using the requirements parser, its much cleaner now!

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.

cool :)

@potiuk
Copy link
Member

potiuk commented Nov 27, 2024

Interesting error. The "get_install_requiresments" is probably not the best idea to use because it adds escaping to " which is needed to produce "pyproject.toml".

dependencies = [
{{- INSTALL_REQUIREMENTS }}
]

Instead you should just use PROVIDER_DEPENDENCIES.get(provider_id)["deps"] @amoghrajesh - that one returns unescaped dependencies.

(and we do not need to us version suffix - this one is merely used to filter out extras - so version suffix does not matter).

@potiuk
Copy link
Member

potiuk commented Nov 27, 2024

Maybe worth to add a comment in get_install_requirements that it escapes the dependencies :).

@amoghrajesh
Copy link
Contributor Author

Interesting error. The "get_install_requiresments" is probably not the best idea to use because it adds escaping to " which is needed to produce "pyproject.toml".

dependencies = [
{{- INSTALL_REQUIREMENTS }}
]

Instead you should just use PROVIDER_DEPENDENCIES.get(provider_id)["deps"] @amoghrajesh - that one returns unescaped dependencies.

(and we do not need to us version suffix - this one is merely used to filter out extras - so version suffix does not matter).

Oh i see, let me correct that!

@amoghrajesh
Copy link
Contributor Author

@potiuk the tests expect a version suffix, do we need it or not at all?

@amoghrajesh
Copy link
Contributor Author

@potiuk i removed the version_suffix related changes. I will do it in a follow up

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

Successfully merging this pull request may close these issues.

Do not add "extra" if provider is already required dependency
4 participants