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

openlineage: migrate OpenLineage provider to V2 facets. #39530

Merged
merged 7 commits into from
Jul 23, 2024

Conversation

JDarDagran
Copy link
Contributor

@JDarDagran JDarDagran commented May 9, 2024

In OpenLineage/OpenLineage#2520 released in openlineage-python==1.13.1 there were introduced V2 classes for facets and events.

@JDarDagran JDarDagran marked this pull request as draft May 9, 2024 19:13
@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch 5 times, most recently from 2e86fb5 to 9947104 Compare May 15, 2024 19:08
@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch from 9947104 to fc72488 Compare May 21, 2024 10:30
@JDarDagran JDarDagran marked this pull request as ready for review May 21, 2024 10:31
@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch 3 times, most recently from 0479252 to e43e266 Compare May 21, 2024 20:44
@JDarDagran
Copy link
Contributor Author

I ran breeze testing tests --upgrade-boto tests/providers/docker in main branch with latest CI image - getting same error results.
Celery integration test seems flaky, no?

@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch from e43e266 to 74bc580 Compare May 21, 2024 21:32
@potiuk
Copy link
Member

potiuk commented May 22, 2024

I ran breeze testing tests --upgrade-boto tests/providers/docker in main branch with latest CI image - getting same error results.

#39747 should fix the docker test in "Latest Botocore".

@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch from 74bc580 to feaa46c Compare May 22, 2024 13:39
@mobuchowski
Copy link
Contributor

@JDarDagran we probably need to bump cross-provider version dependencies. If someone has older OL provider version, and bumps some of the other provider versions, the openlineage methods will fail.

@potiuk
Copy link
Member

potiuk commented May 22, 2024

As discussed with @mobuchowski -> we need to add back-compatibility try/import to past versions of the openlineage provider. We could even potentially add back-compatibility test suite for older version of of openlineage client < 1.13 to test it (symiliar to what we have in Pydantic/Botocore cases.

@JDarDagran
Copy link
Contributor Author

JDarDagran commented Jun 3, 2024

I've added imports with weird-looking structure at first glance:

if TYPE_CHECKING:
    # v2 imports
else:
   try:
       # v2 imports
   except ImportError:
       # v1 imports

This satisfies mypy and works with previous OL provider versions.
I'll be adding backwards-compatibility cross-provider checks in separate PR soon.

@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch from 366b3ed to be583ac Compare June 4, 2024 10:25
@potiuk
Copy link
Member

potiuk commented Jun 8, 2024

Should we re-start discussion about common provider for that one? I have an idea..

@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch from 3d9a6af to 5ca57f8 Compare July 19, 2024 07:55
@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch 4 times, most recently from d128fb6 to 7233d74 Compare July 22, 2024 06:19
Copy link
Contributor

@kacpermuda kacpermuda left a comment

Choose a reason for hiding this comment

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

That's a huge amount of work done, thanks @JDarDagran 🚀 I left some nit comments

@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch from 7233d74 to 2f876b6 Compare July 22, 2024 21:40
@JDarDagran JDarDagran requested a review from potiuk July 22, 2024 21:42
@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch from 2f876b6 to 5383a9c Compare July 22, 2024 22:02
@JDarDagran JDarDagran force-pushed the openlineage/migrate-to-v2-facets branch 2 times, most recently from d92a54c to 0cb2346 Compare July 23, 2024 10:19
@JDarDagran
Copy link
Contributor Author

@potiuk looks like I'm on green path finally consistently 🟢 Would love to get this merged soon as new features arrive and I still need to rebase and apply new imports :)

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>
@mobuchowski mobuchowski merged commit 0206a4c into apache:main Jul 23, 2024
106 of 107 checks passed
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:openlineage AIP-53
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants