Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
HSYZhang
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 841bacce21
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| f"Timeout while waiting for sufficient data for task {task_name}. " | ||
| f"Required: {batch_size}, partition: {partition_id}, fields: {data_fields}" | ||
| ) | ||
| await asyncio.sleep(min(retry_interval, remaining)) |
There was a problem hiding this comment.
When no data arrives, this sleep can consume all of remaining, but the loop then sends another controller request before checking the deadline again. If the controller is slow or becomes unreachable during that extra poll, get_meta can block for the controller RPC timeout (TQ_CONTROLLER_SEND_RECV_TIMEOUT, 200 seconds by default) even though the metadata timeout has already expired; check the deadline at the top of the loop or raise after a sleep that reaches it.
Useful? React with 👍 / 👎.
| "sampling_config": sampling_config, | ||
| } | ||
| deadline = time.monotonic() + TQ_CONTROLLER_GET_METADATA_TIMEOUT | ||
| retry_interval = min(TQ_CONTROLLER_GET_METADATA_CHECK_INTERVAL, TQ_CONTROLLER_GET_METADATA_TIMEOUT / 4) |
There was a problem hiding this comment.
I suggest we use TQ_CONTROLLER_GET_METADATA_CHECK_INTERVAL directly as the retry interval to make it clearer. Users can set up the check interval and timeout to enable retrying, as the default may disable it..
There was a problem hiding this comment.
Understood, I’ve updated the implementation to follow this design.
| #### Low-Level Native API | ||
|
|
||
| The native interfaces of TransferQueue are implemented in `TransferQueueClient`. It offers maximum flexibility through native, atomic operations. | ||
| For `get_meta(mode="fetch")`, the client retries until `TQ_CONTROLLER_GET_METADATA_TIMEOUT` |
There was a problem hiding this comment.
Maybe we don't need to add these doc lines
Signed-off-by: HSYZhang <2908064130@qq.com>
Signed-off-by: HSYZhang <2908064130@qq.com>
|
I'm considering directly remove the blocking logic in controller and use client to choose whether to blocking and wait for the data. I will do the refactor first, please rebase then and only focus on the client side. |
…217) ## Motivation `GET_META` and `NOTIFY_DATA_UPDATE` share the controller’s ZMQ request loop. When a fetch waits for samples, it delays the producer notifications that would mark those samples as ready. ## Changes - Check sample readiness once per fetch and reuse cached sampling results when available. - Remove `polling_mode` from default configs, tutorials, recipes, benchmarks, and E2E fixtures. ## Breaking change With the previous default `polling_mode=False`, `get_meta()` and `async_get_meta()` waited for a batch and raised an error on timeout. They now return an empty `BatchMeta` immediately when no batch is available. Callers that relied on waiting should check `metadata.size == 0` and retry before calling `get_data()`. `StreamingDataset` already handles empty batches and retries. Client-side waiting and automatic retries will be implemented in a follow-up. #212 Remove `controller.polling_mode` from configs and `polling_mode` from controller constructor calls. Both are accepted and ignored in this release, emit a `FutureWarning` when supplied, and will be removed in the next release. ## Validation Compilation, Ruff, and 313 targeted tests passed. The full suite stopped during Ray startup due to local permission restrictions. --------- Signed-off-by: 0oshowero0 <o0showero0@outlook.com> Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
Problem
With polling_mode=False, a GET_META request for insufficient data waited inside the controller’s ZMQ request thread. That same thread handles NOTIFY_DATA_UPDATE, so a producer could write data but its notification could not be processed until the fetch stopped waiting. The default 5-second check interval could also exceed the 1-second metadata timeout.
Changes
Validation