-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Add basic concurrency test for ray client #14630
Conversation
nice! let me try this out.
…On Thu, Mar 11, 2021 at 12:18 PM Eric Liang ***@***.***> wrote:
Assigned #14630 <#14630> to
@richardliaw <https://github.com/richardliaw>.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#14630 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCRZZJESH3HNBZ624Q2F7DTDEJOXANCNFSM4ZA6TK3Q>
.
|
does it make sense to test this?
…On Thu, Mar 11, 2021 at 1:18 PM Richard Liaw ***@***.***> wrote:
nice! let me try this out.
On Thu, Mar 11, 2021 at 12:18 PM Eric Liang ***@***.***>
wrote:
> Assigned #14630 <#14630> to
> @richardliaw <https://github.com/richardliaw>.
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub
> <#14630 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABCRZZJESH3HNBZ624Q2F7DTDEJOXANCNFSM4ZA6TK3Q>
> .
>
|
Actually, I realized this is not needed since the client impl is actually thread-safe (added a simple test). I think your original issue is unrelated. |
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.
LGTM. Not sure though how it "fixes" the other issue (instead of verifying it works).
Sorry, it's actually not fixing the issue (fixed the description). This is just adding another test to avoid regressions on this front. |
time.sleep(1) | ||
|
||
# Can concurrently execute the get. | ||
assert ray.get(fast.remote(), timeout=5) == "ok" |
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.
OOC, will this introduce flakiness in an over-loaded test environment?
The timeout is 5 seconds, which should be pretty generous... depends on how
overloaded the servers are. I can increase it to say 15 if there is a
concern?
…On Fri, Mar 12, 2021 at 3:19 PM Ian Rodney ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In python/ray/tests/test_client.py
<#14630 (comment)>:
> + return "ok"
+
+ class Blocker(threading.Thread):
+ def __init__(self):
+ threading.Thread.__init__(self)
+ self.daemon = True
+
+ def run(self):
+ ray.get(block.remote())
+
+ b = Blocker()
+ b.start()
+ time.sleep(1)
+
+ # Can concurrently execute the get.
+ assert ray.get(fast.remote(), timeout=5) == "ok"
OOC, will this introduce flakiness in an over-loaded test environment?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14630 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAADUSRHEHCD62VRFJMIMHTTDKHPPANCNFSM4ZA6TK3Q>
.
|
@ericl 5 is probably fine, we can always bump in the future |
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.
LGTM
No description provided.