Skip to content

Use __version__ in providers not version #31393

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
Merged
jedcunningham merged 1 commit into from
May 18, 2023
Merged

Use __version__ in providers not version #31393

jedcunningham merged 1 commit into from
May 18, 2023

Conversation

@ashb
Copy link
Member

@ashb ashb commented May 18, 2023 •
edited
Loading

This is for consistency with what we do in airflow and is a common
pattern in Python modules.

As discussed breifly in #30994 (comment)

As per PEP 396:

  1. When a module (or package) includes a version number, the version SHOULD be available in the __version__ attribute
  2. For modules which live inside a namespace package, the module SHOULD include the __version__ attribute. The namespace package itself SHOULD NOT include its own __version__ attribute.

(The namespace package is airflow.providers in our case, so all good.)

TIL there is a pep for this, even if it's now rejected the above bits are still a useful indicator of what is common

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

@ashb
Copy link
Member Author

ashb commented May 18, 2023

cc @eladkal as RM for providers.

@ashb ashb added the allow suspended provider changes Allow changes in suspended providers label May 18, 2023
@ashb ashb closed this May 18, 2023
@ashb ashb reopened this May 18, 2023
jedcunningham
jedcunningham reviewed May 18, 2023
View reviewed changes
@potiuk potiuk added allow suspended provider changes Allow changes in suspended providers and removed allow suspended provider changes Allow changes in suspended providers labels May 18, 2023
eladkal
eladkal approved these changes May 18, 2023
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Thank you Ash!
I will cancel the vote and start RC2 later today

@kaxil kaxil force-pushed the dunder_version_in_providers branch from 2bcd14d to 884717b Compare May 18, 2023 18:56
@kaxil
Copy link
Member

kaxil commented May 18, 2023

Rebased as there was a CI failure on Wait for CI image or something

@potiuk
Copy link
Member

potiuk commented May 18, 2023 •
edited
Loading

This failure is a bug - look closely - openlineage actually imports the renamed version but the import has not been included in the rename. In fact the original version idea came from the __init__.py version that was in openlineage provider.

I would have add a fixup but I can't

@potiuk
Copy link
Member

potiuk commented May 18, 2023 

Traceback:
/usr/local/lib/python3.7/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/providers/openlineage/plugins/test_openlineage_adapter.py:36: in <module>
    from airflow.providers.openlineage.plugins.adapter import _PRODUCER, OpenLineageAdapter
airflow/providers/openlineage/plugins/adapter.py:27: in <module>
    from airflow.providers.openlineage import version as OPENLINEAGE_PROVIDER_VERSION
E   ImportError: cannot import name 'version' from 'airflow.providers.openlineage' (/opt/airflow/airflow/providers/openlineage/__init__.py)

@ashb
Copy link
Member Author

ashb commented May 18, 2023

Yup on it

@ashb
Use __version__ in providers not version
1144b04 
This is for consistency with what we do in `airflow` and is a common
pattern in Python modules.
@jedcunningham
Copy link
Member

jedcunningham commented May 18, 2023

Pushed a fix 👍

@ashb ashb force-pushed the dunder_version_in_providers branch from 72748e1 to 1144b04 Compare May 18, 2023 19:57
@ashb
Copy link
Member Author

ashb commented May 18, 2023

I would have add a fixup but I can't

Curious, I wonder why not
image

(Likely something to do with the policies on the Astronomer GH org I'm sure)

@jedcunningham jedcunningham merged commit abea189 into apache:main May 18, 2023
@jedcunningham jedcunningham deleted the dunder_version_in_providers branch May 18, 2023 20:40
@potiuk
Copy link
Member

potiuk commented May 18, 2023

(Likely something to do with the policies on the Astronomer GH org I'm sure)

Yes. there was a few times I was tempted to make fixup to your PRs and could not :)

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

Reviewers

@eladkal eladkal eladkal approved these changes

@potiuk potiuk potiuk approved these changes

@josh-fell josh-fell josh-fell approved these changes

@jedcunningham jedcunningham jedcunningham approved these changes

@o-nikolas o-nikolas Awaiting requested review from o-nikolas

@Fokko Fokko Awaiting requested review from Fokko

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees

No one assigned

Labels

allow suspended provider changes Allow changes in suspended providers area:providers provider:celery provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:common-sql provider:databricks

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ashb @kaxil @potiuk @jedcunningham @eladkal @josh-fell