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

google: move bigquery openlineage imports inside methods #40062

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

ahidalgob
Copy link
Contributor

closes: #40043

In the latest google providers version (10.19.0), importing bigquery operators results in importing the openlineage package even when the openlineage provider is not installed.

This refactors the code so all imports to the openlineage package happen only inside get_openlineage_facets_on_* again.

#39614 moved the mixin to openlineage/utils.py and this brings it back to operators/bigquery.py to keep the top-level openlineage imports in utils.py, and even worse because the mixin is now bulkier thank before. Another option would be to move BigQueryJobRunFacet and BigQueryErrorRunFacet to another file, and move all openlineage top-level imports in utils.py inside methods. Open to suggestions.


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

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Jun 5, 2024
Copy link
Contributor

@VladaZakharova VladaZakharova left a comment

Choose a reason for hiding this comment

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

LGTM
Great job!

@ahidalgob ahidalgob marked this pull request as draft June 5, 2024 10:20
@ahidalgob ahidalgob force-pushed the fix/bigquery-openlineage branch 2 times, most recently from af12658 to 762eb65 Compare June 5, 2024 11:30
@JDarDagran
Copy link
Contributor

Would be more feasible to keep the code in the separate module as it is and move OL imports from top-level to local imports?

@ahidalgob ahidalgob marked this pull request as ready for review June 5, 2024 12:08
@ahidalgob
Copy link
Contributor Author

Would be more feasible to keep the code in the separate module as it is and move OL imports from top-level to local imports?

@JDarDagran The problem I had trying this was the definition of BigQueryJobRunFacet and BigQueryErrorRunFacet as they need the top level BaseFacet to inherit from. That's why my alternative was to create another file for facets, but doesn't looks clean either.

@JDarDagran
Copy link
Contributor

How about having mixin only in, let's say, airflow/providers/google/cloud/openlineage/mixins.py, in which all imports are local?
I can attempt to fix it in another PR or leave it up to you in this one.

@ahidalgob
Copy link
Contributor Author

sounds good to me, I will make the changes in this PR

@ahidalgob ahidalgob force-pushed the fix/bigquery-openlineage branch 4 times, most recently from 07a3a3b to 0718f3b Compare June 5, 2024 17:12
@potiuk potiuk merged commit fe91596 into apache:main Jun 6, 2024
49 checks passed
fdemiane pushed a commit to fdemiane/airflow that referenced this pull request Jun 6, 2024
jannisko pushed a commit to jannisko/airflow that referenced this pull request Jun 15, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

google providers 10.19.0 bigquery operators directly depends on openlineage package
4 participants