Enhance SFT/DPO reader #226
Merged
Conversation
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
Contributor
There was a problem hiding this comment.
Summary of Changes
Hello @pan-x-c, 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 significantly enhances the SFT (Supervised Fine-Tuning) and DPO (Direct Preference Optimization) data reading capabilities by introducing a new, dedicated Formatter abstraction. This refactoring centralizes data processing logic, making the readers more modular and easier to maintain. Key changes include the removal of the CHATPAIR prompt type and the ReadStrategy enum, simplifying the overall data pipeline and configuration. The update also includes general code cleanup and new test coverage for the introduced formatters.
Highlights
- New Formatter Abstraction: Introduced a new
Formatterabstraction and concrete implementations (SFTMessagesFormatter,DPOPlaintextFormatter, etc.) to centralize and standardize the logic for processing and tokenizing SFT and DPO data. - Data Reader Refactoring: Refactored
SFTDataReaderandDPODataReaderto leverage the newFormatterclasses, significantly simplifying their internal logic by delegating data preparation and tokenization. - Removal of CHATPAIR PromptType: Eliminated the
CHATPAIRprompt type from the system, streamlining the supported data formats for SFT and DPO tasks. - Removal of ReadStrategy: Removed the
ReadStrategyenum and its associated parameters from theBufferReaderinterface and all its implementations, simplifying data retrieval mechanisms. - Configuration Cleanup: Cleaned up the
FormatConfigby removing unused and deprecated fields, improving configuration clarity and maintainability.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Collaborator
Author
|
/unittest-all |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring by creating Formatter classes to handle SFT and DPO data formatting. This greatly simplifies the data reader implementations and improves code organization. The removal of the deprecated CHATPAIR prompt type and the ReadStrategy further streamlines the codebase. The changes to FormatConfig also contribute to better clarity.
I've found a couple of critical issues in the new file_reader.py that would cause runtime errors, and some minor inconsistencies in the new formatter.py file. Please see my detailed comments for suggestions on how to fix them.
Collaborator
Author
|
/unittest-all |
Summary
Tests
Github Test Reporter by CTRF 💚 |
Collaborator
Author
|
/unittest-module-cli |
Summary
Tests
Github Test Reporter by CTRF 💚 |
hiyuchang
approved these changes
Aug 27, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the SFT/DPO data reader by introducing a new Formatter abstraction and cleaning up configuration classes. The changes focus on simplifying data formatting for both Supervised Fine-Tuning (SFT) and Direct Preference Optimization (DPO) workflows while removing deprecated functionality.
- Adds new
Formatterclasses to handle SFT/DPO data formatting with better separation of concerns - Removes deprecated
CHATPAIRPromptType andReadStrategyenum from constants - Cleans up
FormatConfigby removing unused fields and reorganizing remaining ones
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| trinity/common/constants.py | Removes deprecated imports, CHATPAIR enum value, and entire ReadStrategy enum |
| trinity/common/config.py | Reorganizes and removes unused fields from FormatConfig and DataProcessorConfig |
| trinity/cli/launcher.py | Moves import and removes unused log_level parameter from run function |
| trinity/buffer/schema/formatter.py | Introduces new Formatter abstract class and concrete implementations for SFT/DPO data |
| trinity/buffer/reader/sql_reader.py | Removes ReadStrategy parameter from read methods |
| trinity/buffer/reader/queue_reader.py | Removes ReadStrategy parameter and related validation logic |
| trinity/buffer/reader/file_reader.py | Refactors to use new formatter classes and removes inline formatting logic |
| trinity/buffer/ray_wrapper.py | Removes ReadStrategy parameter and simplifies read logic to use LFU only |
| trinity/buffer/buffer_reader.py | Removes ReadStrategy parameter from abstract interface |
| tests/cli/launcher_test.py | Adds comprehensive tests for launcher functionality |
| tests/buffer/formatter_test.py | Adds tests for all new formatter classes |
| examples/dpo_humanlike/dpo.yaml | Updates comment to remove reference to deprecated chatpair option |
| docs/sphinx_doc/source/tutorial/trinity_configs.md | Updates documentation to reflect config changes |
| docs/sphinx_doc/source/tutorial/example_reasoning_basic.md | Removes chatpair from documentation |
| docs/sphinx_doc/source/tutorial/example_dpo.md | Removes chatpair from documentation |
Comments suppressed due to low confidence (2)
trinity/buffer/schema/formatter.py:1
- The variable
self.prompt_typeis not defined in this class. It should bemeta.format.prompt_typebased on the context.
from abc import ABC, abstractmethod
trinity/buffer/reader/file_reader.py:1
- The
_get_assistant_messagemethod is no longer used after the refactoring to use formatters. This method should be removed to avoid dead code.
"""Filed based buffer reader."""
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Formatterto format SFT/DPO data.CHATPAIRPromptTypeReadStrategyfrom buffer readerFormatConfigChecklist
Please check the following items before code is ready to be reviewed.