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

feat: timeout argument for client.sample #133

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: timeout argument for client.sample #133

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7a1dd6
feat: timeout argument for client.sample
varshneydevansh Aug 23, 2023
a138193
corrected with the if-clause
varshneydevansh Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions reverb/cc/sampler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,13 @@ class GrpcSamplerWorker : public SamplerWorker {
}
context_ = std::make_unique<grpc::ClientContext>();
context_->set_wait_for_ready(false);

// Buffer time to account for connection overhead
constexpr auto CONNECTION_BUFFER_TIME = std::chrono::milliseconds(200);

Copy link
Contributor

Choose a reason for hiding this comment

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

The deadline must only be set if the rate_limiter_timeout is set. Also actually it isn't quite this easy I just realised since you also can't set the deadline when using the datasets (which is the recommended way).

Furthermore, there is a whole bunch of tests needed here to verify that all the behaviours are as expected.

Copy link
Author

Choose a reason for hiding this comment

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

You are right, I oversight that if-clause to check if the rate_limiter_timeout is set or not.

When utilizing the “datasets” method. It implies that while using “datasets”, we cannot set a deadline for the RPC call as it might be getting transferred. Am I getting this right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the normal behaviour is to wait for the connection to be establish and only break out if the rate limiter has blocked for the specified amount of time.

Basically I think there might be a need to clearly separate the concepts here and follow a pattern more similar to the writers. I.e. instead of just returning a generator from client.sample we might have to return a special object which has support for getting the next item with an optional deadline.

Really though, this feature will be quite a lot of work and I can't guarantee that I'll be able to accept it unless it is both very cleanly implemented and guaranteed to work in all cases. How important is this feature for you? Unless it is absolutely critical I would probably recommend you not to spend more time on this.

Copy link
Author

Choose a reason for hiding this comment

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

I am just looking for my learning and to improve my programming ability. Since 2017 I did ML/AI stuff but left open source without even trying. Now I am doing what I left and seeking places where I can learn better with some guidance.

So, this is all for my learning. =)

// Setting the deadline for the gRPC context
context_->set_deadline(absl::ToChronoTime(absl::Now() + rate_limiter_timeout + CONNECTION_BUFFER_TIME));

stream = stub_->SampleStream(context_.get());
}

Expand Down