-
Notifications
You must be signed in to change notification settings - Fork 94
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
Less strict numpy and pyarrow dependencies #74
Less strict numpy and pyarrow dependencies #74
Conversation
pyproject.toml
Outdated
{version = ">=10.0.1", python = ">=3.11"} | ||
] | ||
lz4 = "^4.0.2" | ||
requests="^2.18.1" | ||
oauthlib="^3.1.0" | ||
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"}, |
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.
It looks like numpy
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 of pyarrow
requires.
{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"} |
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.
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
@@ -13,15 +13,15 @@ python = "^3.7.1" | |||
thrift = "^0.16.0" | |||
pandas = "^1.3.0" | |||
pyarrow = [ | |||
{version = ">=9.0.0", python = ">=3.7,<3.11"}, | |||
{version = ">=6.0.0", python = ">=3.7,<3.11"}, |
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.
Probably we could allow even older versions but I think this should make most people happy.
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
Signed-off-by: Thomas Newton <thomas.w.newton@gmail.com>
Looks like there is one failure on the CI checks
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. |
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 ( If someone could retry that check and review the PR that would be much appreciated. |
@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. |
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. |
@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 🤦 |
Superseded by #90 |
Currently this library has quite strict requirements for
pyarrow
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 with
poetry build
and using it in my project and it worked as expected.I've had difficulty updating the
poetry.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 Newton thomas.w.newton@gmail.com