-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
source-google-sheets: use poetry for dependency management #34944
source-google-sheets: use poetry for dependency management #34944
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @alafanechere and the rest of your teammates on Graphite |
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
1c5ffcd
to
fef6a42
Compare
f81e501
to
8e7457f
Compare
fef6a42
to
f562554
Compare
8e7457f
to
8a0003a
Compare
f562554
to
390d9d3
Compare
8a0003a
to
7dd4717
Compare
390d9d3
to
574b6a3
Compare
7dd4717
to
8a909b1
Compare
574b6a3
to
9abc45c
Compare
9abc45c
to
65dfb08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from the airbyte-lib side - validated locally and it works as expected
8a909b1
to
1b1e55b
Compare
65dfb08
to
e52fc7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs changes look good to me, but I haven't tested the new commands ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this!
Re: python version — I remember @girarda mentioned that someone reported a bug in the CDK when running a newer Python, and I'm very curious to look at what an upgrade to 3.12 could give us.
This would not be an urgent project, but I would like the cleaner typing syntax, and the performance improvements — and would love to see how much of performance gains we can get overall when the CDK and all connectors and our tools switch.
Another prereq for this might be to start testing connectors with Pythons matrix from 3.9 to 3.12.
@@ -1,67 +1,91 @@ | |||
# Pypi Source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, the previous version here weirds me out. Was it just a completely wrong doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was ...
|
||
[tool.poetry.dependencies] | ||
python = "^3.9" | ||
requests = "==2.31.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why hard pin? I get it if we're just moving from setup.py
as it was, but figured in theory we want to unpin most dependencies to major get at least patch / minor version upgrades for free?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My take is that Poetry.lock
gives us the deterministic build, but the actual dependency list should be as flexible as possible. If it was previously hard-pinned in setup.py, it might be because we were trying to do the lock thing with the deps list. I would try and err on the side of unpinning.
Perhaps, though, this should be another step after this all is done. For example, note that the CDK is pinned. I don't think we should unpin it right now, that might cause issues ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@natikgadzhi the idea there is to force this new connector version to have the same dependency version as the previous one. Even if the previous version was not pinning a version I want to pin main deps in this newest version to guarantee no regression.
These pins can be relaxed without issues in upcoming PRs if needed.
1b1e55b
to
05c08a5
Compare
e52fc7b
to
76acaef
Compare
What
Closes #34954
We want to make our connectors use
poetry
for dependency management (see tech spec)This PR is migrating
source-google-sheets
, a certified connector, topoetry
.The changes were generated by a new airbyte-ci command I'm currently working on (PR to come).
The idea is to get early feedback and address problems on the migration flow if any.
How
pyproject.toml
andpoetry.lock
setup.py
🚨 User Impact 🚨
please note that, unlike
setup.py
based packages, packages managed with poetry must declare the version of python with which they are compatible. I picked^3.9
which means the connector can work on Python 3.9 or higher (3.10).It's a requirement to work with airbyte-lib as:
^3.9
, which means airbyte-lib is not forcing users to use an old python version in their environment.Our base image is still based on Python 3.9.18 which means the released connector will still used a consistent python version.