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

fix: tweak datahub-kafka requirement #110

Merged
merged 2 commits into from
Jan 30, 2024
Merged

fix: tweak datahub-kafka requirement #110

merged 2 commits into from
Jan 30, 2024

Conversation

hsheth2
Copy link
Contributor

@hsheth2 hsheth2 commented Jan 27, 2024

This should enable pydantic 2 support. The acryl-datahub kafka source still requires pydantic v2, but the datahub-kafka sink does not. We only need the sink capabilities here.

Slack ref: https://datahubspace.slack.com/archives/CV2KB471C/p1706298872346799?thread_ts=1706201071.771369&cid=CV2KB471C

Copy link

github-actions bot commented Jan 27, 2024

Unit Test Results (build & test)

63 tests  ±0   63 ✔️ ±0   3s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit d7666d0. ± Comparison against base commit bb11384.

♻️ This comment has been updated with latest results.

@@ -33,7 +33,7 @@ def get_long_description():
acryl_datahub_min_version = os.environ.get("ACRYL_DATAHUB_MIN_VERSION") or "0.12.1.2"

base_requirements = {
f"acryl-datahub[kafka]>={acryl_datahub_min_version}",
f"acryl-datahub[datahub-kafka]>={acryl_datahub_min_version}",
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems kafka and datahub-kafka both used kafka_common from metadata-ingestion/setup.py, how would this change support pydantic 2? I looked at the gradle build log for this PR and it looks like it first pulls pydantic pydantic-2.5.3 and then uninstalls it and installs pydantic-1.10.14 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All plugins except for a couple currently get a pydantic v1 pin https://github.com/datahub-project/datahub/blob/1498c36875450b1a1f44d53e8e8c47c41a91dc69/metadata-ingestion/setup.py#L737

I believe it installs pydantic v1 in the gradle build because it includes the snowflake_tag_propagation action that needs the acryl-datahub snowflake plugin, which needs pydantic v1

However, this should allow other usages of actions to not need pydantic v1

@TonyOuyangGit
Copy link
Contributor

I think this is probably due to the update for lint, without changing those source codes, the build is failing with the following files with lint errors, rerun the `./gradlew lintFix will solve the issue:

would reformat /home/runner/work/datahub-actions/datahub-actions/datahub-actions/src/datahub_actions/pipeline/pipeline_util.py
would reformat /home/runner/work/datahub-actions/datahub-actions/datahub-actions/src/datahub_actions/pipeline/pipeline.py
would reformat /home/runner/work/datahub-actions/datahub-actions/datahub-actions/src/datahub_actions/plugin/action/utils/term_resolver.py
would reformat /home/runner/work/datahub-actions/datahub-actions/datahub-actions/src/datahub_actions/utils/name_resolver.py

@hsheth2 hsheth2 merged commit 680fe19 into main Jan 30, 2024
3 checks passed
@hsheth2 hsheth2 deleted the hsheth2-patch-2 branch January 30, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants