Open
Conversation
Contributor
Summary of ChangesHello, 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 architectural refactoring of the server, merging the functionalities of Tinker and Twinkle into a single, unified system. The changes aim to enhance maintainability and reduce redundancy by centralizing common utilities, establishing a dedicated gateway for client API routing, and modularizing the server's core components into distinct, manageable packages. This new structure simplifies deployment, improves code organization, and sets the foundation for future development. 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the server architecture. By unifying the tinker and twinkle server implementations, it greatly improves code structure, reduces duplication, and enhances maintainability. Key improvements include a unified gateway, model, and sampler application, consolidation of common utilities, and decoupling of client-side data types. The new structure is much cleaner and provides a solid foundation for future development. I've added a couple of minor suggestions to improve code style by moving inline imports to the top of their respective files.
Note: Security Review did not run due to the size of the PR.
Collaborator
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a major and well-executed refactoring of the server architecture, unifying the Tinker and Twinkle protocols under a single gateway. Key improvements include moving resource lifecycle management to be server-side and session-based, introducing typed Pydantic models for API requests and responses, and simplifying the client-side experience with a new TwinkleClient manager. The code is now better organized and more robust. I have one suggestion regarding a client-side dependency on server code.
There was a problem hiding this comment.
Pull request overview
This PR refactors Twinkle’s server/client surface to a unified “gateway + model + sampler + processor” architecture that serves both Tinker-compatible (/tinker/*) and Twinkle-native (/twinkle/*) endpoints, and introduces session-based lifecycle management (session header propagated from client to server) for keeping adapters/processors alive without per-resource heartbeats.
Changes:
- Introduces shared
twinkle_client.typesPydantic models and updates client wrappers to return typed responses. - Replaces the legacy
twinkle.server.twinkle/*andtwinkle.server.tinker/*app modules with unified deployments undertwinkle.server.gateway,twinkle.server.model,twinkle.server.sampler, andtwinkle.server.processor. - Adds session ID propagation (
X-Twinkle-Session-Id) and updates server state/task-queue codepaths to async APIs.
Reviewed changes
Copilot reviewed 104 out of 111 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/twinkle_client/utils/patch_tinker.py | Switches env var naming for the injected tinker client token. |
| src/twinkle_client/types/training.py | Adds shared training-run/checkpoint Pydantic models for Twinkle-native APIs. |
| src/twinkle_client/types/session.py | Adds session create/heartbeat request/response models. |
| src/twinkle_client/types/server.py | Adds basic health/error + checkpoint-path response models. |
| src/twinkle_client/types/sampler.py | Adds sampler request/response Pydantic models (sample, set_template, add_adapter). |
| src/twinkle_client/types/processor.py | Adds processor request/response Pydantic models (create/heartbeat/call). |
| src/twinkle_client/types/model.py | Adds model-management request/response Pydantic models. |
| src/twinkle_client/types/checkpoint.py | Adds ResolvedLoadPath model and moves it into client types for reuse. |
| src/twinkle_client/types/init.py | Re-exports types for convenient import twinkle_client.types as types. |
| src/twinkle_client/sampler/vllm_sampler.py | Updates sampler client routes and returns typed models instead of raw dicts. |
| src/twinkle_client/processor/base.py | Updates processor client routes to new unified processor service paths. |
| src/twinkle_client/http/utils.py | Replaces contextvars with global config + adds session-id support + forces /api/v1 suffix. |
| src/twinkle_client/http/http_utils.py | Adds session header propagation + improves error surfacing with server detail. |
| src/twinkle_client/http/heartbeat.py | Updates processor heartbeat endpoint routing to new unified processor service. |
| src/twinkle_client/http/init.py | Updates exported HTTP helpers to match new utils API (session-id support). |
| src/twinkle_client/dataset/base.py | Updates dataset processor client routes to new unified processor service. |
| src/twinkle_client/dataset/packing_dataset.py | Same as above for packing dataset client. |
| src/twinkle_client/dataset/lazy_dataset.py | Same as above for lazy dataset client. |
| src/twinkle_client/dataset/iterable_packing_dataset.py | Same as above for iterable packing dataset client. |
| src/twinkle_client/dataset/iterable_dataset.py | Same as above for iterable dataset client. |
| src/twinkle_client/dataloader/dataloader.py | Updates dataloader processor client routes to new unified processor service. |
| src/twinkle_client/init.py | Introduces init_twinkle_client() creating a server-side session + background heartbeat. |
| src/twinkle/server/utils/validation.py | Stores session id from request headers and exposes get_session_id_from_request(). |
| src/twinkle/server/utils/task_queue.py | Makes server-state future status updates async + adds schedule_task_and_wait(). |
| src/twinkle/server/utils/state/server_state.py | Converts several ServerStateProxy APIs to async (touch_session, futures, replica availability). |
| src/twinkle/server/utils/processor_manager.py | Adds a session-based processor lifecycle manager mixin with per-token limits. |
| src/twinkle/server/utils/checkpoint_base.py | Renames/reshapes checkpoint persistence base; reuses shared ResolvedLoadPath type. |
| src/twinkle/server/utils/adapter_manager.py | Switches adapter expiry to session-based expiration + adds max lifetime enforcement. |
| src/twinkle/server/utils/init.py | Updates exports to the new module names (checkpoint_base, processor_manager). |
| src/twinkle/server/twinkle/server.py | Removes legacy Twinkle-only gateway server implementation. |
| src/twinkle/server/twinkle/sampler.py | Removes legacy Twinkle-only sampler implementation. |
| src/twinkle/server/twinkle/processor.py | Removes legacy Twinkle-only processor implementation. |
| src/twinkle/server/twinkle/common/transformers_model.py | Removes legacy Twinkle-only compat wrapper (moved into unified backends). |
| src/twinkle/server/twinkle/init.py | Removes legacy lazy-module entrypoint for twinkle server subpackage. |
| src/twinkle/server/tinker/sampler.py | Removes legacy Tinker-only sampler implementation. |
| src/twinkle/server/tinker/common/transformers_model.py | Removes legacy Tinker-only transformers compat wrapper (moved into unified backends). |
| src/twinkle/server/tinker/common/megatron_model.py | Removes legacy Tinker-only megatron compat wrapper (moved into unified backends). |
| src/twinkle/server/tinker/common/io_utils.py | Removes legacy Tinker-only checkpoint IO utilities (replaced by common managers). |
| src/twinkle/server/tinker/common/init.py | Removes legacy tinker common exports. |
| src/twinkle/server/tinker/init.py | Removes legacy lazy-module entrypoint for tinker server subpackage. |
| src/twinkle/server/sampler/twinkle_handlers.py | Adds Twinkle-native sampler endpoints on the unified sampler deployment. |
| src/twinkle/server/sampler/tinker_handlers.py | Adds Tinker-compatible sampler endpoint on the unified sampler deployment. |
| src/twinkle/server/sampler/app.py | Adds unified SamplerManagement deployment wiring both tinker + twinkle routes. |
| src/twinkle/server/sampler/init.py | Exposes build_sampler_app. |
| src/twinkle/server/processor/twinkle_handlers.py | Adds Twinkle-native processor endpoints (create/call) with session-based lifecycle. |
| src/twinkle/server/processor/app.py | Adds unified ProcessorManagement deployment using ProcessorManagerMixin. |
| src/twinkle/server/processor/init.py | Exposes build_processor_app. |
| src/twinkle/server/model/backends/transformers_model.py | Adds unified transformers backend supporting both Tinker and Twinkle I/O styles. |
| src/twinkle/server/model/backends/megatron_model.py | Adds unified megatron backend supporting Tinker I/O style. |
| src/twinkle/server/model/backends/common.py | Centralizes shared backend helpers (collect/metrics cleanup/output shaping). |
| src/twinkle/server/model/backends/init.py | Initializes backend package. |
| src/twinkle/server/model/app.py | Adds unified ModelManagement deployment wiring both tinker + twinkle routes. |
| src/twinkle/server/model/init.py | Exposes build_model_app. |
| src/twinkle/server/gateway/twinkle_gateway_handlers.py | Adds Twinkle-native management endpoints (sessions, runs, checkpoints, weights_info, path). |
| src/twinkle/server/gateway/server.py | Adds unified gateway server deployment wiring both Tinker + Twinkle APIs. |
| src/twinkle/server/gateway/proxy.py | Updates internal proxy routing to hit /tinker/* paths on unified deployments. |
| src/twinkle/server/gateway/init.py | Exposes build_server_app. |
| src/twinkle/server/common/twinkle_checkpoint.py | Refactors Twinkle checkpoint/run managers to use shared client Pydantic models. |
| src/twinkle/server/common/tinker_checkpoint.py | Introduces Tinker checkpoint/run managers using tinker.types. |
| src/twinkle/server/common/serialize.py | Moves/centralizes serialization utilities under twinkle.server.common. |
| src/twinkle/server/common/router.py | Makes replica-capacity query async for compatibility with async ServerStateProxy. |
| src/twinkle/server/common/datum.py | Adds/keeps common datum helpers (copyright header update). |
| src/twinkle/server/common/checkpoint_factory.py | Adds factory for choosing tinker vs twinkle checkpoint managers by client type. |
| src/twinkle/server/common/init.py | Exposes common utilities/factories for server subpackages. |
| src/twinkle/server/main.py | Simplifies CLI help/args toward unified server behavior. |
| src/twinkle/preprocessor/init.py | Adds GSM8KProcessor export. |
| src/twinkle/infra/_ray/ray_helper.py | Adjusts Ray task resource request for helper placement group operation. |
| src/twinkle/hub/hub.py | Modifies hub push behavior to validate success (but changes return behavior). |
| setup.cfg | Extends flake8 ignore list (adds E125). |
| cookbook/transformers/sp_fsdp_dense.py | Updates example model id to Qwen3.5-4B. |
| cookbook/transformers/fsdp2.py | Updates example model id to Qwen3.5-4B. |
| cookbook/client/twinkle/transformer/server_config.yaml | Removes older twinkle-only server config example. |
| cookbook/client/twinkle/transformer/server.py | Removes older twinkle-only server launcher example. |
| cookbook/client/twinkle/megatron/server_config.yaml | Removes older twinkle-only megatron server config example. |
| cookbook/client/twinkle/megatron/server.py | Removes older twinkle-only megatron server launcher example. |
| cookbook/client/twinkle/self_host/self_congnition.py | Updates training loop example to new typed responses / combined step API. |
| cookbook/client/twinkle/self_host/sample.py | Updates sampling example to new typed sampler response. |
| cookbook/client/twinkle/self_host/grpo.py | Updates GRPO example to GSM8K + new typed save/sample/metric APIs. |
| cookbook/client/tinker/self_host/self_cognition.py | Updates import paths to unified twinkle.server.common helpers. |
| cookbook/client/tinker/self_host/sample.py | Adds new tinker-compatible sampling example script. |
| cookbook/client/tinker/self_host/lora.py | Adds new tinker-compatible LoRA training example script. |
| cookbook/client/tinker/modelscope_service/server.py | Removes legacy modelscope-service server launcher example. |
| cookbook/client/tinker/modelscope/self_cognition.py | Updates import paths to unified twinkle.server.common helpers. |
| cookbook/client/tinker/modelscope/sample.py | Adds new modelscope-hosted tinker-compatible sampling example script. |
| cookbook/client/server/transformer/server_config.yaml | Updates tinker-compatible server config and adds processor service app. |
| cookbook/client/server/transformer/server.py | Adds/updates server launcher for cookbook config. |
| cookbook/client/server/megatron/server_config_4b.yaml | Updates megatron server config (processor service added). |
| cookbook/client/server/megatron/server_config.yaml | Updates megatron server config (processor service added). |
| cookbook/client/server/megatron/server.py | Switches default config file to server_config_4b.yaml. |
| README_ZH.md | Updates documentation import paths to unified server common helpers. |
| README.md | Updates documentation import paths to unified server common helpers. |
You can also share your feedback on Copilot code review. Take the survey.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR type
PR information
Write the detail information belongs to this PR.
Experiment results
Paste your experiment result here(if needed).