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

Fix: refactor to adapt to changes to shapely dependency #1376

Merged
merged 7 commits into from
Oct 5, 2022
Merged

Fix: refactor to adapt to changes to shapely dependency #1376

merged 7 commits into from
Oct 5, 2022

Conversation

chalmerlowe
Copy link
Collaborator

@chalmerlowe chalmerlowe commented Oct 4, 2022 •
edited by parthea
Loading

The shapely library has undergone some internal refactoring.
Certain classes, such as WKTReader are no longer used, instead, the library relies on some functions that are imported from what appears to be pygeos.

This change points at the correct function to replace the outdated method call.

  • NEW: shapely.wkt.loads()
  • OLD: shapely.geos.WKTReader(shapely.geos.lgeos).read

Fixes #1364 🦕

Context:

This issue appeared when running a test in python-bigquery-sqlalchemy ... when running certain tests in that library, the code tried to load the tests/unit/testconf.py file from that library which references table.py in this library.

That was causing some failures in tests in the python-bigquery-sqlalchemy library.

@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Oct 4, 2022
@chalmerlowe
Copy link
Collaborator Author

@shollyman can you take a look at this for me and give me a sanity check?

shollyman
shollyman reviewed Oct 4, 2022
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.

I see in the pandas helper we also use the well-known bytes reader, does this also need cleanup?

https://github.com/googleapis/python-bigquery/blob/main/google/cloud/bigquery/_pandas_helpers.py#L66

May be also worth checking system tests for unusual setups.

Are there other imports under shapely that need scrutiny beyond shapely.geos?

@chalmerlowe chalmerlowe changed the title Fix shapely bug Fix: refactor to adapt to changes to shapely dependency Oct 5, 2022
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Oct 5, 2022
@chalmerlowe
Copy link
Collaborator Author

chalmerlowe commented Oct 5, 2022 •
edited
Loading

@shollyman

I checked the remainder of the code for references to shapely, WKT*, WKB*, etc.

Added a minor change based on what I found.

@chalmerlowe chalmerlowe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 5, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 5, 2022
@shollyman
Copy link
Contributor

There's no test changes? Is this indicative that our existing tests were sufficient in catching and exercising this dependency? Do we need to exercise anything new as part of the changes?

@chalmerlowe
Copy link
Collaborator Author

There's no test changes? Is this indicative that our existing tests were sufficient in catching and exercising this dependency? Do we need to exercise anything new as part of the changes?

The existing tests were already using the new refactoring > they referred to the correct functions in the correct files.

@chalmerlowe chalmerlowe marked this pull request as ready for review October 5, 2022 17:48
@chalmerlowe chalmerlowe requested a review from a team October 5, 2022 17:48
@chalmerlowe chalmerlowe requested a review from a team as a code owner October 5, 2022 17:48
@chalmerlowe chalmerlowe merged commit 2afd278 into main Oct 5, 2022
@chalmerlowe chalmerlowe deleted the fix-shapely-bug branch October 5, 2022 18:17
@release-please release-please bot mentioned this pull request Oct 5, 2022
Merged
@parthea parthea added the release-please:force-run To run release-please label Oct 10, 2022
@release-please release-please bot removed the release-please:force-run To run release-please label Oct 10, 2022
@chalmerlowe chalmerlowe mentioned this pull request Nov 3, 2022
Merged
This was referenced Nov 8, 2022
Closed
Closed
Closed
@kenwade4 kenwade4 mentioned this pull request Dec 15, 2022
Merged
abdelmegahedgoogle pushed a commit to abdelmegahedgoogle/python-bigquery that referenced this pull request Apr 17, 2023
@chalmerlowe @abdelmegahedgoogle
Fix: refactor to adapt to changes to shapely dependency (googleapis#1376
43040cf 
)

* fix: refactored to account for changes in dependency

* Removes comment and ensures linting success

* refactor to use loads() function

* fix: refactors to account for changes to shapely dependency

* fix: refactors to account for changes to shapely dependency

* blacken the code

* add mypy ignore flag for shapely import
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

Assignees
No one assigned
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. size: s Pull request size is small.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: Missing attribute: WKTReader
4 participants
@chalmerlowe @shollyman @parthea @yoshi-kokoro