refactor(inference): extract reusable RemoteInferenceGenerator - #1911
refactor(inference): extract reusable RemoteInferenceGenerator#1911dyurk-lila wants to merge 7 commits into
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the MoE router replay mechanism and optimizes inference server communication by introducing a dedicated RemoteInferenceGenerator and a compact base64-encoded payload format for routed expert indices. It also adds a router_padding_mask to training batches and refactors token metadata alignment to correctly handle padding and context-parallel sharding. The reviewer feedback highlights critical improvements: ensuring the _generator field is properly cleared and reset during serialization/deserialization in RemoteInferenceClient, falling back to a default value instead of raising a ValueError for non-finite logprobs in vllm_server_actor.py, and handling list-type unwrapped models when virtual pipeline parallelism is enabled in replay_utils.py to prevent runtime crashes.
7350f2c to
46bfb19
Compare
46bfb19 to
0efd49c
Compare
Serializing routed-expert IDs as nested JSON lists creates a large HTTP payload and substantial Python/JSON overhead before training sees the data. - Compact routed-expert IDs to the smallest safe uint8, int16, or int32 dtype. - Base64-encode the contiguous buffer alongside its shape and dtype, and decode it back into a validated compact NumPy array on the client. - Serialize the vLLM response and parse client responses with orjson. - Gate packed routes behind the existing routed-expert request flag. - Accept the decoded NumPy route arrays at preprocessing, validating dtype and shape before padding and tensor conversion. Because orjson emits JSON null rather than raising for non-finite floats, the sampled-token logprob path floors missing and non-finite values to -9999.0 -- the same floor vLLM applies at its own serving boundaries. This keeps one bad logprob from failing the whole generate request. Note the isfinite test also catches NaN, which vLLM's max(logprob, -9999.0) misses since max returns its first argument on a False comparison. This change is intentionally scoped to transport and preprocessing: it does not alter batching, Experience, device placement, or backend replay setup.
0efd49c to
070fbe3
Compare
Under pipeline parallelism each rank replays only its local router layers, but the Megatron worker eagerly expanded the full global-layer routed-expert tensor to int32 before replay setup, allocating a large device temporary for unused layers. Keep routed-expert IDs in their compact dtype through whole-batch device movement, index_select the current PP stage's router layers before metadata alignment, and perform the single int32 conversion inside _split_replay_indices so only the bounded PP-local slice is materialized as int32. Also validate the 4D replay-indices shape up front. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract the single-request HTTP generation path out of RemoteInferenceClient into a standalone RemoteInferenceGenerator and a RemoteGenerateResult dataclass. RemoteInferenceClient now owns an internal generator and delegates session management, _post, and _generate_single to it. This is a pure refactor with no functionality change: endpoint routing, retry/backoff, cache_salt handling, serialization, and lifecycle behavior are all preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
070fbe3 to
a68fedb
Compare
Resolve conflicts in favor of main, which carries the reviewed squash of PR NovaSky-AI#1909 (42f3d44) that this branch had based on its pre-squash form (a81e61d). Conflicts taken from main: - inference_servers/{remote_inference_client,vllm_server_actor}.py: main consolidated logprobs_wire.py + routed_experts_wire.py into generate_wire.py. - utils/routed_experts.py, train/dataset/preprocess.py and its test: main narrows wide routed-expert dtypes instead of rejecting them, and builds padded routes via make_replay_padding_indices_np. Also dropped the superseded logprobs_wire.py / routed_experts_wire.py modules and their tests (coverage lives in main's test_generate_wire.py and utils/test_routed_experts.py), retargeted a stale wire import in test_remote_inference_client.py, and removed a duplicated RoutedExpertIndices import in skyrl_gym_generator.py. replay_utils.py keeps this branch's local-layer route expansion, which supersedes main's pre-NovaSky-AI#1910 post-split layer mapping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nerator Bring main into the next branch in the stack via the merged PR NovaSky-AI#1910 branch rather than merging main directly: main is already a parent of that merge, so this inherits it transitively while reusing the PR NovaSky-AI#1909 squash conflict resolutions instead of re-deriving them. Merged without conflicts. The notable incoming change to this branch's own file is remote_inference_client.py's routed-expert wire import, which main moved from routed_experts_wire.py into the consolidated generate_wire.py; git combined that with this branch's rewrite of the same file correctly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RemoteInferenceGenerator imported RoutedExpertIndices from skyrl.utils.routed_experts, which does not exist -- skyrl/utils/ holds only log.py, storage.py and tok.py. The module lives at skyrl.backends.skyrl_train.utils.routed_experts. This predates the merge: the import is wrong at the branch's own head (a68fedb), where it makes remote_inference_client.py, and therefore test_remote_inference_client.py, fail to import at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR NovaSky-AI#1910 landed in main as squash commit 7188a1d while this stack was being prepared, so the branch's pre-squash copy of that work now collides with main's reviewed version -- the same squash-vs-pre-squash situation NovaSky-AI#1909 created for NovaSky-AI#1910. Conflict in replay_utils.py resolved in favor of main, which differs from this branch's copy only by a three-line comment added in review explaining the dense-layer mismatch fallback. Nothing from this branch is lost. Also picks up e2cddc5 (rollout-anchored CISPO under fully-async training). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the remote inference client by extracting the HTTP request and token generation logic into a new RemoteInferenceGenerator class and introducing a RemoteGenerateResult dataclass. The RemoteInferenceClient now delegates its generation tasks to this new generator. Feedback is provided regarding the loss of a protective try...except block around session.close() during the refactoring of the session closure logic, which could lead to unhandled exceptions during cleanup.
| async def aclose(self) -> None: | ||
| if self._session is not None and not self._session.closed: | ||
| await self._session.close() | ||
| self._session = None |
There was a problem hiding this comment.
The protective try...except block around session.close() was lost during the refactoring into RemoteInferenceGenerator.aclose(). Since aclose() is called during cleanup and teardown paths (such as in __aexit__), any exception raised during session closure could propagate and disrupt the cleanup process or mask other errors. We should restore the try...except block to safely handle and log any exceptions during session closure.
| async def aclose(self) -> None: | |
| if self._session is not None and not self._session.closed: | |
| await self._session.close() | |
| self._session = None | |
| async def aclose(self) -> None: | |
| if self._session is not None and not self._session.closed: | |
| try: | |
| await self._session.close() | |
| except Exception as exc: | |
| logger.warning(f"Encountered exception {exc} while closing client session") | |
| self._session = None |
Summary
Pure refactor (no functionality change) that extracts the single-request HTTP
generation path out of
RemoteInferenceClientso it can be reused on its own:RemoteInferenceGeneratorand aRemoteGenerateResultdataclass.RemoteInferenceClientown an internalRemoteInferenceGeneratoranddelegate session management,
_post, and_generate_singleto it.the full inference/control-plane client.
cache_salthandling,serialization, and lifecycle behavior.
Testing
uv run --isolated --extra dev --extra fsdp pytest tests/backends/skyrl_train/inference_servers/test_remote_inference_client.py(58 passed)
ruffandblackclean on the changed files.