-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
update grpc to 1.44.0 #22384
update grpc to 1.44.0 #22384
Conversation
Can you merge the latest master? |
@rkooo567 should I rebase from master again or are the tests we see known to be flaky? |
Test failures should be unrelated. |
@mwtian is the Mac cpp test unrelated? It seems a bit weird to me |
Yeah, I didn't see the Mac C++ tests earlier. Their failures seem problematic. I will be OOO next week. If we haven't fixed it then, I can take a look. |
@mwtian sorry to ping, but if you can glance at the mac test or have anyone else that has a mac machine test it out that would be wonderful. |
@rkooo567 mentioned he will take a look, but I can give it a try tomorrow too. |
Yeah I had no time for this (maybe until the end of next week) |
This is the same issue as when we upgraded grpc last time. Now it is happening for all tests.
I think we should upgrade the CI MacOS version higher then 10.15 Catalina. |
There are problems with running C++ tests in MacOS 10.15 Catalina, when upgrading to the newest grpc due to dynamic linking: #22384 (comment). The problem does not exist for Python tests in Catalina, or in C++ tests of other systems. Upgrading MacOS CI from Catalina is also blocked in the short term: ray-project/reef#24 (comment) So working around the issue by using static linking for C++ tests on Mac.
Thanks @mwtian I hadn't realized there was a blocker on updating the macos ci version or else I wouldn't have pushed so hard for this, especially considering this PR itself is not needed for getting py3.10 support (which is what I'm currently trying to resolve) |
Oh just saw it got merged, let me rebase to restart the ci on this PR. |
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.
All tests passed, after rerunning one Windows test failure.
Thanks again for the patch! |
Seeing these errors for building Windows wheels on
|
Riiip, let me open up a PR and test out a patch. The solution is probably just a simple change from |
Great, thanks for looking into this and fixing the local path! |
Why are these changes needed?
Updates grpc to 1.44.0 to remove local patch needed for grpc to build.
EDIT: there have been changes to how python is found (mostly removing python2 support) and as such the local python-patch we have for grpc needs to be modified.
This time contributing it to upstream (grpc/grpc#28895) so that it'll get added in a newer version!
For anyone that comes across this:
Here is the error itself for why we need the
grpc-python.patch
file: https://buildkite.com/ray-project/ray-builders-pr/builds/24659#d293616f-225d-41f9-8de2-03780f12b13f/2386-2416Related issue number
Similar in spirit to #21866
GRPC changelog: https://github.com/grpc/grpc/releases/tag/v1.44.0
Checks
scripts/format.sh
to lint the changes in this PR.Relevant People:
@rkooo567