fix: enhance replay_buffer example.#450
Conversation
Signed-off-by: Klaus Ma <klausm@nvidia.com>
There was a problem hiding this comment.
Code Review
This pull request introduces parallel sampling for the RL replay buffer example, enabling sample requests to be distributed across multiple service instances through a new sample_parallelism configuration. The implementation includes logic to split batch requests, updated CLI options, and revised documentation. Feedback from the review emphasizes that autoscale=True must be explicitly set for the warmup parameter to correctly instantiate multiple service instances, a detail currently overlooked in the code and README. Additionally, the reviewer advised against the deletion of test_replay_buffer.py, as it removes vital unit tests for the buffer's incremental deserialization logic.
I am having trouble creating individual review comments. Click here to see my feedback.
examples/rl/replay_buffer/main.py (81)
The rr.service call for an instance defaults autoscale to False. According to the RunnerService documentation in sdk/python/src/flamepy/runner/runner.py (lines 151-153), when autoscale is False, the service creates exactly one instance and the warmup parameter is ignored. To achieve the intended parallelism with multiple service instances, autoscale=True should be explicitly provided.
buffer_svc = rr.service(buffer, autoscale=True, warmup=sample_parallelism)
examples/rl/replay_buffer/README.md (13)
This code snippet should include autoscale=True. As documented in the SDK's RunnerService class, the warmup parameter is only effective when autoscale is enabled; otherwise, the service defaults to a single instance.
buffer_svc = rr.service(buffer, autoscale=True, warmup=sample_parallelism)
examples/rl/replay_buffer/README.md (144)
The statement that warmup=N creates N service instances for a fixed-size service (where autoscale=False) contradicts the SDK documentation. According to RunnerService in sdk/python/src/flamepy/runner/runner.py, autoscale=False creates exactly one instance, and warmup is only used when autoscale=True. The example and its description should be updated to use autoscale=True to support multiple instances.
examples/rl/replay_buffer/test_replay_buffer.py (1-114)
The removal of test_replay_buffer.py eliminates unit tests for the ReplayBuffer deserialization logic. Since this logic is still present and critical for the incremental update pattern described in the README, these tests should be retained or updated to reflect the changes rather than being deleted.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Klaus Ma <klausm@nvidia.com>
No description provided.