Skip to content

fix: make pyarrow an optional dependency post-3.20.0 yanked release#1879

Merged
tswast merged 4 commits intomainfrom
issue1877-pyarrow-optional
Mar 28, 2024
Merged

fix: make pyarrow an optional dependency post-3.20.0 yanked release#1879
tswast merged 4 commits intomainfrom
issue1877-pyarrow-optional

Conversation

@tswast
Copy link
Contributor

@tswast tswast commented Mar 28, 2024

Partial rollback of 0ac6e9b

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1877 🦕

@tswast tswast requested a review from a team March 28, 2024 14:40
@tswast tswast requested review from a team as code owners March 28, 2024 14:40
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Mar 28, 2024
leahecole
leahecole reviewed Mar 28, 2024
View reviewed changes
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Mar 28, 2024
@tswast tswast requested a review from leahecole March 28, 2024 15:45
@shollyman shollyman requested review from Linchin, chalmerlowe and shollyman and removed request for agrawal-siddharth March 28, 2024 15:53
@chalmerlowe
Copy link
Collaborator

chalmerlowe commented Mar 28, 2024

@tswast

We appear to have two conflicting versions of pyarrow:

The noxfile.py file identifies these versions of Python:

Line 40: UNIT_TEST_PYTHON_VERSIONS = ["3.7", "3.8", "3.12"]

Later when we reference

Line 125: UNIT_TEST_PYTHON_VERSIONS[0]

we are implicitly referencing version 3.7 (at least until the definition of this constant changes) AND we then tell the system to install version 1.0.0 of pyarrow.
Line 126: session.install("pyarrow==1.0.0")

But in testing/constraints-3.7.txt we list the pyarrow version as

Line 30: pyarrow==3.0.0

QUESTION: should those two match?
QUESTION: if we are making pyarrow optional, why in the unit_noextras session are we requiring pyarrow?

@tswast
Copy link
Contributor Author

tswast commented Mar 28, 2024

QUESTION: should those two match?

No. They should not. testing/constraints-3.7.txt covers when pyarrow is installed as part of our package installation, e.g. unit-3.7. This doesn't apply to the unit_noextras-3.7 session.

QUESTION: if we are making pyarrow optional, why in the unit_noextras session are we requiring pyarrow?

There is a comment explaining exactly this. See: #933 Requiring the correct version of pyarrow to be installed broke some environments where pyarrow was stuck on an older version. We need to be robust to this failure mode.

@tswast
Copy link
Contributor Author

tswast commented Mar 28, 2024

See: https://pip.pypa.io/en/stable/user_guide/#constraints-files for more information on constraints files. When applied, it affects what version of a package is installed but it doesn't affect packages that aren't included because of a transitive dependency of the package being installed.

@tswast tswast closed this Mar 28, 2024
@tswast tswast reopened this Mar 28, 2024
Linchin
Linchin approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@Linchin Linchin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I will not merge for now, waiting for Chalmer to see the response.

@chalmerlowe
Copy link
Collaborator

chalmerlowe commented Mar 28, 2024

Thanks @tswast
Took a look at the write-up on constraints.txt files.
My understanding is:

  • unlike a requirements.txt file, it does NOT require that a specific package be installed
  • HOWEVER, if that package DOES get installed (maybe as a dependency of something else OR as an optional package), it is constrained to what is allowed in the constraints.txt file.

@tswast
Copy link
Contributor Author

tswast commented Mar 28, 2024

That's correct. Note that we are installing pyarrow without a constraints file in unit_noextras and pyarrow is not included as a dependency in any subsequent installations, so the constraints do not apply.

@tswast tswast changed the title fix: make pyarrow an optional dependency again fix: make pyarrow an optional dependency post-3.20.0 yanked release Mar 28, 2024
chalmerlowe
chalmerlowe approved these changes Mar 28, 2024
View reviewed changes
Copy link
Collaborator

@chalmerlowe chalmerlowe left a comment

Choose a reason for hiding this comment

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

LGTM

@chalmerlowe chalmerlowe mentioned this pull request Mar 28, 2024
Closed
shollyman
shollyman approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

Generally, looks like a good unwinding of the behavior changes and test configurations. I did add one question around test philosophy that may be outside the scope of this PR specifically.

tests/unit/test__versions_helpers.py
pyarrow is not None,
reason="pyarrow is installed, but this test needs it not to be",
)
def test_try_import_raises_error_w_no_pyarrow():
Copy link
Contributor

@shollyman shollyman Mar 28, 2024

Choose a reason for hiding this comment

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

Is there a way of asserting that a test like this was run at least once across multiple sessions? Without that, we could still potentially swallow this signal if the precondition is never met.

Copy link
Contributor Author

@tswast tswast Mar 28, 2024

Choose a reason for hiding this comment

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

Coverage should catch that. We check coverage on test files as well as the core library.

Copy link
Contributor Author

@tswast tswast Mar 28, 2024

Choose a reason for hiding this comment

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

See:

python-bigquery/noxfile.py

Line 97 in 7dfee0c

"--cov=tests/unit",

@tswast tswast merged commit 21714e1 into main Mar 28, 2024
@tswast tswast deleted the issue1877-pyarrow-optional branch March 28, 2024 20:15
@release-please release-please bot mentioned this pull request Mar 28, 2024
Merged
@tswast tswast mentioned this pull request Mar 28, 2024
Closed
This was referenced Apr 9, 2024
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shollyman shollyman shollyman approved these changes

@chalmerlowe chalmerlowe chalmerlowe approved these changes

@Linchin Linchin Linchin approved these changes

@parthea parthea Awaiting requested review from parthea parthea is a code owner automatically assigned from googleapis/python-samples-owners

@leahecole leahecole Awaiting requested review from leahecole

Assignees

No one assigned

Labels

api: bigquery Issues related to the googleapis/python-bigquery API. size: m Pull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pyarrow import error in 3.20.0

5 participants

@tswast @chalmerlowe @shollyman @leahecole @Linchin