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

refactor(sql): remove sqlalchemy from the codebase #8074

Merged
merged 1 commit into from
Feb 3, 2024
Merged

refactor(sql): remove sqlalchemy from the codebase #8074

merged 1 commit into from
Feb 3, 2024

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Jan 23, 2024 •
edited
Loading

This PR removes sqlalchemy from the codebase.

@cpcloud cpcloud added refactor Issues or PRs related to refactoring the codebase sqlalchemy SQLAlchemy-based backends sql Backends that generate SQL tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split` tes-required-for-merge Issues that must addressed before merging the-epic-split branch into main labels Jan 23, 2024
@cpcloud cpcloud requested a review from kszucs January 23, 2024 13:17
@cpcloud cpcloud mentioned this pull request Jan 30, 2024
Merged
@cpcloud cpcloud force-pushed the nuke-sqlalchemy branch 2 times, most recently from 3f7d544 to a89cea5 Compare January 31, 2024 10:33
@cpcloud cpcloud added this to the 9.0 milestone Jan 31, 2024
@cpcloud cpcloud removed the tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split` label Jan 31, 2024
@cpcloud
Copy link
Member Author

cpcloud commented Jan 31, 2024

As part of this I discovered that pydruid has an inadvertent hard dependency on sqlalchemy: druid-io/pydruid#313

I've also submitted a PR there to address the issue. If this doesn't get merged before the 9.0 release then it's probably time to consider removing the druid backend.

@jcrist
Copy link
Member

jcrist commented Jan 31, 2024

If this doesn't get merged before the 9.0 release then it's probably time to consider removing the druid backend.

Wouldn't that only affect those that installed ibis-framework[druid]? Why is pydruid having a sqlalchemy dependency a blocker?

@cpcloud
Copy link
Member Author

cpcloud commented Jan 31, 2024

It's not a blocker technically speaking, except that we still have to list sqlalchemy somewhere.

Perhaps it is enough to pass the sqlalchemy extra to pydruid.

@cpcloud
Copy link
Member Author

cpcloud commented Jan 31, 2024 •
edited
Loading

Ok, that seems to work and not cause a huge number of headaches AFAICT but I am still a bit 😒 🤣

@cpcloud cpcloud marked this pull request as ready for review January 31, 2024 16:58
@cpcloud cpcloud requested a review from jcrist January 31, 2024 17:09
jcrist
jcrist approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@jcrist jcrist left a comment

Choose a reason for hiding this comment

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

Nice

@cpcloud
Copy link
Member Author

cpcloud commented Jan 31, 2024

Ok, I've had enough of it with quoting. I'm going True.

@cpcloud
Copy link
Member Author

cpcloud commented Feb 1, 2024

The remaining duckdb failures are due to quoting, which should be fixed after #8172 is merged into the-epic-split and then this PR is rebased on the-epic-split.

@kszucs kszucs force-pushed the the-epic-split branch 3 times, most recently from 432d151 to e4df99b Compare February 2, 2024 11:42
@cpcloud cpcloud force-pushed the nuke-sqlalchemy branch 4 times, most recently from 3514106 to e1e10d6 Compare February 3, 2024 10:55
@cpcloud cpcloud force-pushed the nuke-sqlalchemy branch 6 times, most recently from 5dd75af to 78ab2dc Compare February 3, 2024 12:22
@cpcloud
Copy link
Member Author

cpcloud commented Feb 3, 2024

Clouds are passing:

❯ pytest -m 'bigquery or snowflake' -n auto --dist loadgroup --snapshot-update
======================================================================================== test session starts =========================================================================================
platform linux -- Python 3.10.13, pytest-8.0.0, pluggy-1.4.0
benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
Using --randomly-seed=516386949
rootdir: /home/cloud/src/ibis
configfile: pyproject.toml
plugins: hypothesis-6.97.4, xdist-3.5.0, cov-4.1.0, benchmark-4.0.0, randomly-3.15.0, mock-3.12.0, httpserver-1.0.8, repeat-0.9.3, snapshot-0.9.0, clarity-1.0.1
16 workers [3241 items]
..x...x..x......x..x.......x.xs..............sx.x.x..x...x...ssssssssssssssssssssss....xx.....x.x....x.xxx..x.x...s..x...........x.....x.xxx..x....x..........xx.....x...x......x.x....xx.x..x [  5%]
..x.............x.x.........x..x...xx..x...x...x..xx.x...xx..........xx...x..x........x.x.x...xx.x.x.x..xxx.xxxx.x.xxx.xxx.xxx.x.x..xxx..x.xx...x....x...x...x......xxx.xx..x.x.....x......... [ 11%]
x....x.....x..x.x.x..xx...x.x.x....x..........x...............x......x.........................x...xx........x............x..x.......x...x.x..x...xx...x..xx.x........................x.x..... [ 17%]
.x...x....x.........x...x.x...x......................x..........................................................................x...x..x...............x...............x.........x.......xx... [ 23%]
..........................xx................................x..........................................................x...x.x............x..x.....x...............x.......x................s. [ 29%]
........s.....x...........x....xxx.x............x.............sx.x.......xx..x...x.xx..x...........x...x........x..x..x...x.x..x..x....x.x.x........................x...xx..xx...xxxxxxxxxxxx. [ 35%]
xxx.x..x.xx........x..xxx.xx..x....x.....x.....x.x...x........x....s...............x...............xx....x......x......x...........xx.........x.................x......x..................x... [ 41%]
x..........x.........xx..............x.........s........x.....................x...............x........x..........x......................x...............................x...x......x......... [ 46%]
......xx.....................x....x..............x.x.............xx....x..x...........x......x..................................x...........x................xx..............xx............x.. [ 52%]
...x..x.......x....x............x....xx..xx...x....x............x..xxxx....xx......x.........x.......................xx...............................x..................x.................... [ 58%]
..s......x..xx....................x.............................................................x..x................x...x.x....x..........................x................................x.. [ 64%]
..................x........x.....x..................x.x.xx..x...x................x.........x..x..x.......s......s..........................s..x....xx...................x..x...x.x............ [ 70%]
.........x.........x............x....x.x..........xx..xx....x.x................x.........xxxx...x.xxx...xxx...xxxxxxxxxxxx..x..x.x.x..x......x........x...x....x...xxx.x.x.x....x..x.x.x..x.x. [ 76%]
..x.x..x....................x..x.x.....s..................xx..x..............x.....x..x.......x...........x..x........x.....x.xs...........x...x....x.....xx......x.......................x... [ 82%]
.............................................................................................................................................................................................. [ 87%]
.............s.........................x...................................................................................................x.................................................. [ 93%]
..........................................s..................................................................................................x................x............................... [ 99%]
...........                                                                                                                                                                                    [100%]
===================================================================== 2788 passed, 38 skipped, 415 xfailed in 389.91s (0:06:29) ======================================================================

@cpcloud
Copy link
Member Author

cpcloud commented Feb 3, 2024

The DuckDB windows failure is a transient network issue during installation, I'll restart the job once the others finish, and then merge this PR.

@cpcloud cpcloud merged commit 463df8e into ibis-project:the-epic-split Feb 3, 2024
72 checks passed
@cpcloud cpcloud deleted the nuke-sqlalchemy branch February 3, 2024 13:33
@kszucs
Copy link
Member

kszucs commented Feb 3, 2024

Nice! Thanks @cpcloud!

cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 4, 2024
@cpcloud
refactor(sql): remove sqlalchemy from the codebase (ibis-project#8074)
409db95
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 5, 2024
@cpcloud
refactor(sql): remove sqlalchemy from the codebase (ibis-project#8074)
b2cfaff
kszucs pushed a commit that referenced this pull request Feb 5, 2024
@cpcloud @kszucs
refactor(sql): remove sqlalchemy from the codebase (#8074)
34fc2e1
kszucs pushed a commit that referenced this pull request Feb 6, 2024
@cpcloud @kszucs
refactor(sql): remove sqlalchemy from the codebase (#8074)
d3469f6
kszucs pushed a commit that referenced this pull request Feb 6, 2024
@cpcloud @kszucs
refactor(sql): remove sqlalchemy from the codebase (#8074)
8d5f68e
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
@cpcloud
refactor(sql): remove sqlalchemy from the codebase (ibis-project#8074)
9ac548e
cpcloud added a commit that referenced this pull request Feb 12, 2024
@cpcloud
refactor(sql): remove sqlalchemy from the codebase (#8074)
3ce81ee
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
@cpcloud
refactor(sql): remove sqlalchemy from the codebase (ibis-project#8074)
323e0f0
cpcloud added a commit that referenced this pull request Feb 12, 2024
@cpcloud
refactor(sql): remove sqlalchemy from the codebase (#8074)
b9fba4e
kszucs pushed a commit that referenced this pull request Feb 12, 2024
@cpcloud @kszucs
refactor(sql): remove sqlalchemy from the codebase (#8074)
22004ed
ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Feb 21, 2024
@cpcloud @ncclementi
refactor(sql): remove sqlalchemy from the codebase (ibis-project#8074)
a458ca1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcrist jcrist jcrist approved these changes

@kszucs kszucs Awaiting requested review from kszucs

Assignees
No one assigned
Labels
refactor Issues or PRs related to refactoring the codebase sql Backends that generate SQL sqlalchemy SQLAlchemy-based backends tes-required-for-merge Issues that must addressed before merging the-epic-split branch into main
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cpcloud @jcrist @kszucs