Conversation
Summary of ChangesHello @Yunnglin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant overhaul of the client-server interaction and its accompanying documentation within the project. It establishes a robust mechanism for automatically generating client-side Python wrappers, thereby reducing manual effort and promoting code consistency. Concurrently, it delivers comprehensive, structured documentation and practical Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the documentation and examples for the client-server architecture. It refactors the cookbook examples out of the legacy directory, adding extensive comments and step-by-step instructions to make them much clearer for users. A comprehensive new documentation section has been added, explaining the different client/server modes and backends. The client-side library is now auto-generated and included in the repository, with the generator updated to add warnings against manual edits. The changes are well-structured and greatly enhance the project's usability and maintainability. I have no specific comments as the changes are of high quality.
There was a problem hiding this comment.
Pull request overview
This PR updates the client/server documentation and cookbook examples, and starts checking in the auto-generated twinkle_client HTTP wrappers (previously ignored) while removing legacy cookbook artifacts.
Changes:
- Add auto-generated
twinkle_clientmodules (model/sampler/dataset/dataloader/processor/reward) and their__init__.pyexports. - Refresh cookbook configs/scripts (new structured examples) and delete legacy cookbook client examples.
- Update docs to include the new “服务端和客户端” section, and adjust the client generator to prepend an auto-generated warning header.
Reviewed changes
Copilot reviewed 48 out of 50 changed files in this pull request and generated 51 comments.
Show a summary per file
| File | Description |
|---|---|
| src/twinkle_client/sampler/vllm_sampler.py | New HTTP sampler wrapper (auto-generated). |
| src/twinkle_client/sampler/init.py | Export sampler wrapper. |
| src/twinkle_client/reward/math_reward.py | New HTTP reward wrapper (auto-generated). |
| src/twinkle_client/reward/init.py | Export reward wrapper. |
| src/twinkle_client/processor/base.py | New HTTP input processor wrapper (auto-generated). |
| src/twinkle_client/processor/init.py | Export processor wrapper. |
| src/twinkle_client/model/multi_lora_transformers.py | New HTTP model wrapper (auto-generated). |
| src/twinkle_client/model/init.py | Export model wrapper. |
| src/twinkle_client/dataset/packing_dataset.py | New HTTP dataset wrapper (auto-generated). |
| src/twinkle_client/dataset/lazy_dataset.py | New HTTP dataset wrapper (auto-generated). |
| src/twinkle_client/dataset/iterable_packing_dataset.py | New HTTP iterable dataset wrapper (auto-generated). |
| src/twinkle_client/dataset/iterable_dataset.py | New HTTP iterable dataset wrapper (auto-generated). |
| src/twinkle_client/dataset/base.py | New base HTTP dataset wrapper (auto-generated). |
| src/twinkle_client/dataset/init.py | Export dataset wrappers. |
| src/twinkle_client/dataloader/dataloader.py | New HTTP dataloader wrapper (auto-generated). |
| src/twinkle_client/dataloader/init.py | Export dataloader wrapper. |
| src/twinkle/sampler/vllm_sampler.py | Switch LoRA patching call style to __call__. |
| src/twinkle/sampler/vllm_engine.py | Whitespace-only cleanup. |
| docs/source/index.rst | Add 服务端和客户端 docs index to toctree. |
| cookbook/legacy/client/twinkle/transformer/server_config.yaml | Remove legacy example. |
| cookbook/legacy/client/twinkle/transformer/server.py | Remove legacy example. |
| cookbook/legacy/client/twinkle/transformer/grpo_lora.py | Remove legacy example. |
| cookbook/legacy/client/twinkle/megatron/server_config.yaml | Remove legacy example. |
| cookbook/legacy/client/twinkle/megatron/server.py | Remove legacy example. |
| cookbook/legacy/client/twinkle/megatron/lora.py | Remove legacy example. |
| cookbook/legacy/client/tinker/transformer/server_config.yaml | Remove legacy example. |
| cookbook/legacy/client/tinker/transformer/server.py | Remove legacy example. |
| cookbook/legacy/client/tinker/transformer/sample.py | Remove legacy example. |
| cookbook/legacy/client/tinker/megatron/server_config.yaml | Remove legacy example. |
| cookbook/legacy/client/tinker/megatron/server.py | Remove legacy example. |
| cookbook/client/twinkle/transformer/server_config.yaml | Add documented Twinkle transformers server config example. |
| cookbook/client/twinkle/transformer/server.py | Add documented Twinkle transformers server launcher. |
| cookbook/client/twinkle/transformer/lora.py | Expand comments/documentation in training example. |
| cookbook/client/twinkle/megatron/server_config.yaml | Add documented Twinkle megatron server config example. |
| cookbook/client/twinkle/megatron/server.py | Add documented Twinkle megatron server launcher. |
| cookbook/client/twinkle/megatron/lora.py | Add documented megatron LoRA training example. |
| cookbook/client/tinker/transformer/server_config.yaml | Add documented Tinker-compatible server config example. |
| cookbook/client/tinker/transformer/server.py | Add documented Tinker-compatible server launcher. |
| cookbook/client/tinker/transformer/self_congnition.py | Improve inline documentation and flow in example script. |
| cookbook/client/tinker/transformer/sample.py | Add documented sampling example. |
| cookbook/client/tinker/transformer/lora.py | Expand comments/documentation in training example. |
| cookbook/client/tinker/megatron/server_config.yaml | Add documented Tinker-compatible megatron server config. |
| cookbook/client/tinker/megatron/server.py | Add documented Tinker-compatible megatron server launcher. |
| cookbook/client/tinker/megatron/lora.py | Expand comments/documentation and add sampling step. |
| client_tools/client_generator.py | Prepend AUTO_GEN_WARNING to generated files/__init__.py. |
| .gitignore | Stop ignoring generated src/twinkle_client/* subpackages so they can be committed. |
Comments suppressed due to low confidence (1)
cookbook/client/tinker/transformer/self_congnition.py:3
- Filename and module name use
self_congnition(typo). Consider renaming toself_cognition.pyto improve discoverability and avoid perpetuating the misspelling in imports/links.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from twinkle.dataset import Dataset | ||
| from twinkle.dataset import DatasetMeta | ||
| from .base import Dataset | ||
|
|
There was a problem hiding this comment.
Same shadowing issue as other dataset wrappers: importing Dataset from twinkle.dataset and then importing Dataset from .base overwrites the first import. Please drop the unused import or rename one of them to avoid ambiguity.
| from torch.utils.data import IterableDataset | ||
|
|
||
| class IterableDataset(IterableDataset): | ||
| """Client wrapper for IterableDataset that calls server HTTP endpoints.""" | ||
|
|
There was a problem hiding this comment.
class IterableDataset(IterableDataset): reuses the same name as the imported base class (torch.utils.data.IterableDataset), which makes the code hard to read/debug and can confuse tooling. Import the torch base class under an alias (e.g., TorchIterableDataset) and subclass that instead.
| - name: models-Qwen2.5-7B-Instruct | ||
| route_prefix: /models/Qwen/Qwen2.5-7B-Instruct # REST path for this model | ||
| import_path: model | ||
| args: | ||
| use_megatron: false # Use HuggingFace Transformers (not Megatron) | ||
| model_id: "ms://Qwen/Qwen2.5-0.5B-Instruct" # ModelScope model identifier to load | ||
| adapter_config: |
There was a problem hiding this comment.
The model service name/route refer to Qwen2.5-7B-Instruct, but model_id is set to ms://Qwen/Qwen2.5-0.5B-Instruct. This mismatch is likely to confuse users and can lead to loading a different model than the config name suggests; align the name/route_prefix with the actual model_id (or update the comments to clearly state it's intentionally using 0.5B).
| - name: models-Qwen2.5-7B-Instruct | ||
| route_prefix: /models/Qwen/Qwen2.5-7B-Instruct # REST path for this model | ||
| import_path: model | ||
| args: | ||
| use_megatron: true # Use Megatron-LM backend (not HuggingFace) | ||
| model_id: "ms://Qwen/Qwen2.5-0.5B-Instruct" # ModelScope model identifier to load | ||
| nproc_per_node: 2 # Number of GPU processes per node |
There was a problem hiding this comment.
Same model naming mismatch as the Transformers config: models-Qwen2.5-7B-Instruct / route prefix indicates 7B, but model_id points to the 0.5B variant. Please make these consistent to avoid misleading configuration/docs.
| sampler_code = AUTO_GEN_WARNING + '''from typing import Any, Optional, List, Dict, Union | ||
| import uuid | ||
| from twinkle_client.http import TWINKLE_SERVER_URL | ||
| from twinkle_client.http import http_post, heartbeat_manager |
There was a problem hiding this comment.
The sampler_code template defined here generates VLLMSampler.__init__ with a return response.json(). That makes the generated client invalid (constructors must return None) and will also fail when /create returns a non-JSON/empty body. Please adjust the template to not return from __init__ and to only parse JSON for endpoints that actually return JSON.
| def __del__(self): | ||
| try: | ||
| heartbeat_manager.unregister_processor(self.processor_id) | ||
| except: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| def __del__(self): | ||
| try: | ||
| heartbeat_manager.unregister_processor(self.processor_id) | ||
| except: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| """Cleanup: unregister adapter from heartbeat manager.""" | ||
| try: | ||
| heartbeat_manager.unregister_adapter(self.adapter_name) | ||
| except: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| def __del__(self): | ||
| try: | ||
| heartbeat_manager.unregister_processor(self.processor_id) | ||
| except: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| try: | ||
| if self.adapter_name: | ||
| heartbeat_manager.unregister_adapter(self.adapter_name) | ||
| except: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
No description provided.