databricks/databricks-sql-pythonPublic

NotificationsYou must be signed in to change notification settings
Fork126
Star210

Less strict numpy and pyarrow dependencies#74

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

Tom-Newton wants to merge2 commits intodatabricks:mainfromTom-Newton:tomnewton/less_strict_pip_dependencies

Closed

Less strict numpy and pyarrow dependencies#74

Tom-Newton wants to merge2 commits intodatabricks:mainfromTom-Newton:tomnewton/less_strict_pip_dependencies

Conversation

Copy link

Contributor

Tom-Newton commentedDec 15, 2022•
edited
Loading

Currently this library has quite strict requirements forpyarrow andnumpy versions which significantly limits the usefulness. As far as I can tell this library does not require any particular features ofnumpy orpyarrow added in recent versions.

Resolves:#55

I tested building the wheel locally withpoetry build and using it in my project and it worked as expected.

I've had difficulty updating thepoetry.lock file though. I've never usedpoetry before but I think I need to runpoetry update. Unfortunately this seems to be taking an extremely long time (> 20 minutes) to the point that I think it must be broken.

Signed-off-by: Thomas Newtonthomas.w.newton@gmail.com

Tom-Newton commented

Dec 15, 2022

View reviewed changes

pyproject.toml Outdated

		numpy = [
		{version ="1.21.1",python =">=3.7,<3.8"},
		{version ="1.23.4",python =">=3.8"}
		{version =">=1.16.6",python =">=3.7,<3.8"},

Copy link

ContributorAuthor

Tom-NewtonDec 15, 2022

Choose a reason for hiding this comment

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

It looks likenumpy is barely used in this library so I think pretty much any version should work. I chose>=1.16.6 because that is what the latest version ofpyarrow requires.

Tom-Newton commented

Dec 15, 2022

View reviewed changes

pyproject.toml

		{version ="1.21.1",python =">=3.7,<3.8"},
		{version ="1.23.4",python =">=3.8"}
		{version =">=1.16.6",python =">=3.7,<3.11"},
		{version =">=1.23.4",python =">=3.11"}

Copy link

ContributorAuthor

Tom-NewtonDec 15, 2022

Choose a reason for hiding this comment

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

It looks like the special requirement for python 3.11 was added recently in#60 so I assume that is important but I changed it to accept >= to this version.

I also think we should only enforce this requirement on python 3.11

Tom-Newton commented

Dec 15, 2022

View reviewed changes

pyproject.toml

		pandas ="^1.3.0"
		pyarrow = [
		{version =">=9.0.0",python =">=3.7,<3.11"},
		{version =">=6.0.0",python =">=3.7,<3.11"},

Copy link

ContributorAuthor

Tom-NewtonDec 15, 2022

Choose a reason for hiding this comment

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

Probably we could allow even older versions but I think this should make most people happy.

Tom-Newton changed the title~~Less strict dependencies~~Less strict numpy and pyarrow dependencies

Dec 15, 2022

Tom-Newton marked this pull request as ready for review

December 15, 2022 20:46

Tom-Newton requested review fromarikfr,moderakh,susodapop andyunbodeng-db ascode owners

December 15, 2022 20:46

Tom-Newton mentioned this pull request

Dec 31, 2022

PyArrow version requirement#55

Closed

Tom-Newton added2 commits

February 13, 2023 21:36

Less strict dependencies

b57d4ea

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

Fix python version condition

f48ad07

Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>

Copy link

ContributorAuthor

Tom-Newton commentedFeb 13, 2023

Looks like there is one failure on the CI checks

Download action repository 'actions/checkout@v2' (SHA:dc323e67f16fb5f7663d20ff7941f27f5809e9b6)Warning: Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/dc[32](https://github.com/databricks/databricks-sql-python/actions/runs/4168062376/jobs/7214961091#step:1:36)3e67f16fb5f7663d20ff7941f27f5809e9b6'. Error: Resource temporarily unavailable (api.github.com:443)Warning: Back off 22.447 seconds before retry.Warning: Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/dc323e67f16fb5f7663d20ff7941f27f5809e9b6'. Error: Resource temporarily unavailable (api.github.com:443)Warning: Back off 24.765 seconds before retry.Error: Resource temporarily unavailable (api.github.com:443)

I don't think this can be because of my changes. I guess its just a transient thing but I don't think I can re-trigger it.

Copy link

ContributorAuthor

Tom-Newton commentedFeb 13, 2023•
edited
Loading

Thanks for triggering the checks@susodapop. It looks like all the unittests passed for all the different python versions. One of the code quality checks is failing though. From the error (Error: Resource temporarily unavailable (api.github.com:443)) I would guess a retry would solve it.

If someone could retry that check and review the PR that would be much appreciated.

Copy link

ContributorAuthor

Tom-Newton commentedFeb 20, 2023

@susodapop sorry to be impatient but please could you review this? I think this is about to start causing us some significant problems if its not fixed.

Copy link

Contributor

susodapop commentedFeb 22, 2023

Not impatient at all! We just needed to freeze our dependencies to get v2.4.0 out the door. Else this would have merged already.

Copy link

Contributor

susodapop commentedFeb 22, 2023

@Tom-Newton can you grant me push on your fork? I need to re-generate the poetry.lock file before I can merge this but I can't push my version to your fork right now.

Also this reminds me that I need to add a segment to our CONTRIBUTING.md file about updating the project dependencies. Unless you regenerate the poetry.lock file then none of the changes are actually picked up during our unit/e2e tests 🤦