-
Notifications
You must be signed in to change notification settings - Fork 298
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
deps: expand supported pyarrow versions to v4 #643
Conversation
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.
How did you determine that PyArrow 4 was acceptable? When I run nox on your branch, it uses PyArrow 3. This is a bit of a process question, which I'll be faced with sqlalchemy soon.
Samples, which cover typical use cases, now run with v4 and the tests pass. Also, by default |
Are you sure? :) How can you tell? When I run full nox locally pyarrow 3 is installed (except for Python 3.6 which uses 1). What do you get when you run:
? |
If
The output of
However, I did notice that
For some reason different versions are installed in a nox session than when creating a fresh environment manually, hmm... Update:
|
I removed my .nox directory and then ran
IDK how nox+pip picks versions. At some point, I thought someone said we somehow arranged to pick minimal versions, but I don't see anything like that in how pip is invoked and I've been generally perplexed by versions chosen. Again, I'm not really trying to say anything is wrong. I'm just trying to understand our approach. |
@jimfulton I noticed the same, got the version Blocking until we get to the bottom of this. |
How are you running the samples tests? Is that with I'm getting pyarrow 3 for snippets. BTW, I agree wrt expected pip behavior. |
I got version 4 when using the snippets' own noxfile:
I think we could actually benefit to run |
Me too. I got pyarrow 4 when running nox from the snippets directory. |
Maybe in cases like this, we should add a constraint. When I added |
Constraints to force the very latest versions to be used in tests? Might work, although that can still bite us when, say, the renovate bot updates a version pin, but we forget about the constraint that restricts it to a less recent version. I feel that's actually too likely to happen... |
I was suggesting adding Maybe tomorrow I'll ask you to explain how renovate bot works. ATM, I'm not impressed. :) |
@jimfulton Do you feel strongly about adding |
I would add it to protect us. It doesn't harm anything. Remember we had an uncaught bug that this would have caught. I plan to do the same when making sure we test the sqlalchemy dialect with 1.4. |
Makes sense, will add it right now then. |
Closes #635
PR checklist: