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.

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

Airbyte CDK: fix file-based deps #36695

Merged
merged 1 commit into from Apr 1, 2024
Merged

Airbyte CDK: fix file-based deps #36695

merged 1 commit into from Apr 1, 2024

Conversation

artem1205
Copy link
Collaborator

@artem1205 artem1205 commented Mar 29, 2024 •
edited

What

fix deps for file based CDK
Reason:
pdf2image and "pdfminer.six" are missing

How

move packages to [tool.poetry.dependencies]

@artem1205
Airbyte CDK: file based fix deps
04675e0 
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
@vercel Vercel
Copy link

vercel bot commented Mar 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Mar 29, 2024 7:31pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Mar 29, 2024
@artem1205 artem1205 marked this pull request as ready for review March 29, 2024 20:32
@artem1205 artem1205 requested a review from a team as a code owner March 29, 2024 20:32
@artem1205 artem1205 changed the title Airbyte CDK: file based fix deps Airbyte CDK: fix file-based deps Mar 29, 2024
natikgadzhi
natikgadzhi reviewed Mar 29, 2024
View reviewed changes
Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

I think previously the unit tests were still passing, right?

Can you please add the tests that would use these dependencies, so we don't end up in this situation again?

@artem1205
Copy link
Collaborator Author

artem1205 commented Apr 1, 2024 •
edited

@natikgadzhi ,

I think previously the unit tests were still passing, right?

Airbyte CDK tests use command in CI : poetry install --all-extras, that is why we don't see any problems with deps.

Can you please add the tests that would use these dependencies, so we don't end up in this situation again?

We already have CAT/ integration tests in connectors that use CDK deps, see https://github.com/airbytehq/airbyte/actions/runs/8480072489/job/23235149671 as example.

Could you please create an issue with description for tests?

erohmensing
erohmensing approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@erohmensing erohmensing 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 for tracking this down!

airbyte-cdk/python/pyproject.toml
@@ -62,8 +64,6 @@ datamodel_code_generator = "0.11.19"
freezegun = "*"
mypy = "*"
pandas = "2.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

pandas is the only other one on this list that i'm suspicious about being a "dev-only" dependency. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@erohmensing , I'm ok with this. The only line with pandas I found is:

airbyte/airbyte-cdk/python/unit_tests/sources/file_based/in_memory_files_source.py

Line 16 in eb31e4d

import pandas as pd

@artem1205 artem1205 merged commit 4431347 into master Apr 1, 2024
34 checks passed
@artem1205 artem1205 deleted the artem1205/airbyte-cdk-file-based-fix-deps branch April 1, 2024 17:41
nurikk pushed a commit to nurikk/airbyte that referenced this pull request Apr 4, 2024
@artem1205 @nurikk-sa
Airbyte CDK: fix file-based deps (airbytehq#36695)
2f4ee10 
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
markcusack pushed a commit to markcusack/airbyte that referenced this pull request Apr 9, 2024
@artem1205
Airbyte CDK: fix file-based deps (airbytehq#36695)
378b252 
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erohmensing erohmensing erohmensing approved these changes

@tolik0 tolik0 Awaiting requested review from tolik0

@natikgadzhi natikgadzhi Awaiting requested review from natikgadzhi

Assignees
No one assigned
Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@artem1205 @natikgadzhi @erohmensing @octavia-squidington-iii