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

docs(samples): explicitly add bq to samples reqs, upgrade grpc to fix bug on m1 #1290

Merged
merged 3 commits into from
Jul 19, 2022
Merged

docs(samples): explicitly add bq to samples reqs, upgrade grpc to fix bug on m1 #1290

merged 3 commits into from
Jul 19, 2022

Conversation

leahecole
Copy link
Contributor

Related to #1260 and #1262

Can confirm these linked issues definitely happen with M1, but is fixed if you specifically add grpc as an explicit dependency

@leahecole leahecole requested a review from a team July 15, 2022 19:14
@leahecole leahecole requested review from a team as code owners July 15, 2022 19:14
@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. samples Issues that are directly related to samples. labels Jul 15, 2022
@leahecole leahecole requested a review from loferris July 15, 2022 19:14
@leahecole leahecole changed the title explicitly add bq to samples reqs, upgrade grpc to fix bug on m1 fix: explicitly add bq to samples reqs, upgrade grpc to fix bug on m1 Jul 15, 2022
@conventional-commit-lint-gcf
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@leahecole leahecole force-pushed the update_samples_requirements branch from b94dff5 to 3058a2b Compare July 15, 2022 19:14
loferris
loferris suggested changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@loferris loferris left a comment

Choose a reason for hiding this comment

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

Hi @leahecole, looking at the system test failures, I think we can fix this if we update the required versions in setup.py (project root) and the constraint files for at least 3.8 and 3.10 in the testing folder (project root).

@parthea parthea changed the title fix: explicitly add bq to samples reqs, upgrade grpc to fix bug on m1 docs(samples): explicitly add bq to samples reqs, upgrade grpc to fix bug on m1 Jul 16, 2022
@parthea parthea requested a review from loferris July 16, 2022 15:00
@parthea
Copy link
Contributor

parthea commented Jul 16, 2022

Should we also update the grpc requirements in setup.py ? https://github.com/googleapis/python-bigquery/blob/main/setup.py#L32

@leahecole
Copy link
Contributor Author

Hi @leahecole, looking at the system test failures, I think we can fix this if we update the required versions in setup.py (project root) and the constraint files for at least 3.8 and 3.10 in the testing folder (project root).

To clarify, the constraint being the grpc version, not the BQ version we also added, right, @loferris?

loferris
loferris suggested changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@loferris loferris left a comment

Choose a reason for hiding this comment

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

Looking at the Kokoro failures, I'm seeing a flag to update the 3.7 constraints file (in the testing folder) to have the correct grpcio version.
Screen Shot 2022-07-18 at 1 33 21 PM
.

@loferris
Copy link
Contributor

Hi @leahecole, looking at the system test failures, I think we can fix this if we update the required versions in setup.py (project root) and the constraint files for at least 3.8 and 3.10 in the testing folder (project root).

To clarify, the constraint being the grpc version, not the BQ version we also added, right, @loferris?

From what I can tell from the Kokoro test failures, that seems right to me!!

@loferris
Copy link
Contributor

@leahecole double-checked, and you probably want to update the grpcio dependency version in constraints-3.6.txt as well as in constraints-3.7.txt just to be on the safe side.

@parthea
Copy link
Contributor

parthea commented Jul 19, 2022

testing/constraints-3.6.txt could be deleted since we're no longer supporting python 3.6

@leahecole
Copy link
Contributor Author

Ok! I'll make those changes. Thanks for the explanation - I've never touched these constraints files before so I wanted to be super sure I was doing the right thing 😁

@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 Jul 19, 2022
@leahecole leahecole requested a review from loferris July 19, 2022 15:24
parthea
parthea reviewed Jul 19, 2022
View reviewed changes
testing/constraints-3.10.txt
@@ -0,0 +1 @@
grpcio==1.47.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we remove this line as we still want to test the latest version of grpcio in one of the constraints files.

Suggested change
grpcio==1.47.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we had to add this explicitly to get the system tests (if not, would be happy to delete!!), but I'd suggest I make a follow up issue to clean up the versioning across different versions.

loferris
loferris approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@loferris loferris left a comment

Choose a reason for hiding this comment

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

I'm going to approve and merge, but I do want to follow up with @parthea about versioning overall.

@loferris loferris merged commit 9b7e3e4 into main Jul 19, 2022
@loferris loferris deleted the update_samples_requirements branch July 19, 2022 16:49
@release-please release-please bot mentioned this pull request Jul 19, 2022
Merged
@leahecole
Copy link
Contributor Author

SGTM - let me know what you end up deciding!

waltaskew pushed a commit to waltaskew/python-bigquery that referenced this pull request Jul 20, 2022
@leahecole @waltaskew
docs(samples): explicitly add bq to samples reqs, upgrade grpc to fix…
0dd16db 
… bug on m1 (googleapis#1290)

* fix: explicitly add bq to samples reqs, upgrade grpc to fix bug on m1

* update grpc in setup.py

* fix: rm 3.6 constraints, add grpcio to 3.7-3.10 constraints
This was referenced Aug 2, 2022
Closed
Closed
Closed
abdelmegahedgoogle pushed a commit to abdelmegahedgoogle/python-bigquery that referenced this pull request Apr 17, 2023
@leahecole @abdelmegahedgoogle
docs(samples): explicitly add bq to samples reqs, upgrade grpc to fix…
fd1f796 
… bug on m1 (googleapis#1290)

* fix: explicitly add bq to samples reqs, upgrade grpc to fix bug on m1

* update grpc in setup.py

* fix: rm 3.6 constraints, add grpcio to 3.7-3.10 constraints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parthea parthea parthea approved these changes

@loferris loferris loferris approved these changes

@shollyman shollyman Awaiting requested review from shollyman

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

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

Successfully merging this pull request may close these issues.
3 participants
@leahecole @parthea @loferris