-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Widening range of grpcio versions allowed. #28623
Conversation
@jjyao what is the best way to test the |
Yea, I would use conda for that. |
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.
stamp
Could you pull the master head? Want to make sure failed tests are unrelated. |
0193a19
to
1dd22fb
Compare
Notes on windows tests:
|
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.
hmm, we should still set the upper bound? for core dependencies, we should only approve a version until it's proven innocent.
cc @jjyao |
@scv119 This is not the strategy we are following due to the nature of Ray as a library (if you look at setup.py, we don't have upper bounds for most of (core) dependencies). Ray shouldn't prevent users from using the latest versions of dependencies (they may want to use that for some reason like bug fix) that might come out after Ray's release. |
hmm this (setting upper bound) has been the practical strategy we have been following for grpcio right (and other core dependencies)? Empirically, for grpcio, not setting upper bound has caused serious problems (i.e. ray hangs), comparing to the issues where users are not able to upgrade to the latest grpcio. |
grpcio doesn't have upper bound by default as well. I think what we did in the past was forcing an upper bound when we discover an issue and lift the upper bound afterwards after the issue is fixed. (this is what we normally do for other core dependencies as well). The strategy I mentioned is a general strategy we apply to all of our dependencies. Whether we want to make an exception for grpcio is a separate story but I don't think it should apply to all dependencies. Personally I still feel we shouldn't put an upper bound (we may put an upper bound on the major version number not but the minor version number if they are following semantic versioning). @richardliaw may have more insights on this. |
that's said, i'm not comfortable follow the existing practices where we don't set grpcio upperbound, without other protection mechanism.
i think the safest way is to set the upperbound to the version we know that's working, and figure out a less strict protection mechansim. |
Given we haven't reached an agreement on whether to put a cap or not. Let's add a cap to merge this PR since it's still strictly better than Ray 2.0 and we can discuss long term solution later. |
Sounds good. There are more non-flaky Windows tests failing, taking note here so see if they repeat:
Running things manually on a windows machine:
|
9d19ac3
to
6e73e97
Compare
Signed-off-by: Cade Daniel <[email protected]>
Signed-off-by: Cade Daniel <[email protected]>
…ilds Signed-off-by: Cade Daniel <[email protected]>
This reverts commit 1dd22fbd02b95d4c2941178675d136dd8f2b85fa. Signed-off-by: Cade Daniel <[email protected]>
6e73e97
to
4c470e6
Compare
Is there a way I can replicate the BK environment and iterate faster? It is taking forever to wait for BK same error message for both grpc versions
|
Thanks for working on this! Just to understand - besides the missing timeout in the tests, there were no other changes necessary to support grpc 1.48? |
Yep! I believe we prohibited this version because it was causing the hang; since 1.48.0 was yanked and 1.48.1 fixes the issue, we can simply allow it. |
I'm increasing the timeouts on all of the tests that I've seen become flaky on Windows because of this change.
|
Signed-off-by: Cade Daniel <[email protected]>
41927b6
to
183b68f
Compare
Signed-off-by: Cade Daniel <[email protected]>
I have a lint fix but waiting for windows builds to succeed before I push |
windows tests passed |
* Widening range of grpcio versions allowed. Signed-off-by: Cade Daniel <[email protected]> Signed-off-by: Weichen Xu <[email protected]>
See #27299 for context.
script.py
Testing with the following code:
python/grpcio versions tested on above script
test code