Skip to content

Conversation

@chenyushuo
Copy link
Collaborator

@chenyushuo chenyushuo commented Jan 21, 2026

Description

  1. Move Selector into TaskFileReader.
  2. [Breaking Change] Rename update function to feedback in Selector and TaskFileReader.

Checklist

Please check the following items before code is ready to be reviewed.

  • Code has passed all tests
  • Docstrings have been added/updated in Google Style
  • Documentation has been updated
  • Code is ready for review

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @chenyushuo, 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 refactors the architecture of task selection within the trinity buffer system by centralizing the Selector logic directly within the TaskFileReader. This change streamlines the TasksetScheduler by offloading the responsibility of managing task selection to the individual readers, promoting better encapsulation and a clearer separation of concerns. The modification ensures that each TaskFileReader can independently handle how it retrieves tasks, either sequentially or through a configured selector, leading to a more modular and maintainable design.

Highlights

  • Selector Logic Relocation: The core logic for task selection, previously managed by TasksetScheduler, has been moved into the TaskFileReader class, making TaskFileReader responsible for its own selection mechanism.
  • Simplified TasksetScheduler: TasksetScheduler no longer directly instantiates or manages a separate list of selector objects, simplifying its internal structure and reducing its coupling with selection details.
  • Removed Redundant Methods: The read_with_indices and read_with_indices_async methods have been removed from both TaskFileReaderWrapper and TaskFileReader as the selection logic is now integrated into the primary read method.
  • Enhanced TaskFileReader Capabilities: TaskFileReader now includes an update method to propagate pipeline metrics to its internal selector, and its read, state_dict, and load_state_dict methods have been updated to conditionally delegate to the selector if one is configured.
  • Configuration Handling: A deepcopy is now used when creating taskset_config in TasksetScheduler to ensure that modifications (like disabling task selection for specific readers) do not affect other configurations.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the code to move the Selector logic from TasksetScheduler into TaskFileReader. This is a good architectural change that improves modularity by making TaskFileReader responsible for its own data selection strategy, and simplifying TasksetScheduler. The changes are well-executed and consistent across the modified files. I have one minor suggestion regarding an incorrect type hint.

@chenyushuo
Copy link
Collaborator Author

/unittest-module-buffer

@chenyushuo
Copy link
Collaborator Author

/unittest-module-explorer

@chenyushuo
Copy link
Collaborator Author

/unittest-module-buffer

@chenyushuo
Copy link
Collaborator Author

/unittest-module-explorer

@github-actions
Copy link

Summary

Tests 📝 Passed ✅ Failed ❌ Skipped ⏭️ Other ❓ Flaky 🍂 Duration ⏱️
48 48 0 0 0 0 2m

Tests

Test Name Status Flaky Duration
tests/buffer/experience_pipeline_test.py::TestExperiencePipeline::test_experience_pipeline 11.3s
tests/buffer/experience_pipeline_test.py::TestExperiencePipeline::test_pass_rate_calculation 7.2s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_experience_buffer 3.7s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_storage_0_sft 4.9s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_storage_1_dpo 5.4s
tests/buffer/file_test.py::TestFileBuffer::test_file_reader 440ms
tests/buffer/file_test.py::TestFileBuffer::test_file_writer 1.9s
tests/buffer/formatter_test.py::TestFormatter::test_dpo_messages_formatter 594ms
tests/buffer/formatter_test.py::TestFormatter::test_dpo_plaintext_formatter 473ms
tests/buffer/formatter_test.py::TestFormatter::test_multi_modal_sft_formatter 918ms
tests/buffer/formatter_test.py::TestFormatter::test_sft_messages_formatter 1.0s
tests/buffer/formatter_test.py::TestFormatter::test_sft_plaintext_formatter 755ms
tests/buffer/formatter_test.py::TestFormatter::test_task_formatter 239ms
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_buffer_reuse 6.5s
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_capacity 2.5s
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_reuse_count_control 4.7s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_0_queue 3.5s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_1_priority_queue 3.5s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_capacity 4.0s
tests/buffer/reader_test.py::TestBufferReader::test_buffer_reader_registration 543ms
tests/buffer/reward_shaping_mapper_test.py::TestRewardShapingMapper::test_basic_usage 7ms
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_0::test_default_queue_default_sample_strategy 2.3s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_0::test_default_queue_staleness_control_sample_strategy 2.0s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_0::test_priority_queue_default_sample_strategy 2.0s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_0::test_priority_queue_staleness_control_sample_strategy 2.2s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_0::test_sql_staleness_control_sample_strategy 4.8s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_1::test_default_queue_default_sample_strategy 2.0s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_1::test_default_queue_staleness_control_sample_strategy 2.2s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_1::test_priority_queue_default_sample_strategy 2.0s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_1::test_priority_queue_staleness_control_sample_strategy 2.0s
tests/buffer/sample_strategy_test.py::ExperienceStorageTest_1::test_sql_staleness_control_sample_strategy 4.0s
tests/buffer/sql_test.py::TestSQLBuffer::test_sql_exp_buffer_read_write_0 6.0s
tests/buffer/sql_test.py::TestSQLBuffer::test_sql_exp_buffer_read_write_1 2.8s
tests/buffer/sql_test.py::TestSQLBuffer::test_sql_task_buffer_read_write 3.3s
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_0 72ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_1 54ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_2 88ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_3 89ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_4 88ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_5 92ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_6 107ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_simple 44ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_0_file 60ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_1_sql 3.4s
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_2_file 40ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_3_sql 3.1s
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_4_file 38ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_5_sql 4.0s

Github Test Reporter by CTRF 💚

@github-actions
Copy link

Summary

Tests 📝 Passed ✅ Failed ❌ Skipped ⏭️ Other ❓ Flaky 🍂 Duration ⏱️
48 47 0 1 0 0 12m 30s

Skipped

Tests Status
tests/explorer/workflow_test.py::TestAgentScopeWorkflowAdapter::test_adapter_v1 skipped ⏭️

Tests

Test Name Status Flaky Duration
tests/explorer/explorer_test.py::TestExplorerCountdownEval::test_explorer 1m 28s
tests/explorer/explorer_test.py::TestExplorerGSM8KRULERNoEval::test_explorer 1m 27s
tests/explorer/explorer_test.py::TestExplorerGSM8k::test_explorer 3m 38s
tests/explorer/explorer_test.py::ServeTest::test_serve 1m 25s
tests/explorer/proxy_test.py::RecorderTest::test_recorder 64ms
tests/explorer/scheduler_test.py::SchedulerTest::test_async_workflow 5.9s
tests/explorer/scheduler_test.py::SchedulerTest::test_concurrent_operations 5.4s
tests/explorer/scheduler_test.py::SchedulerTest::test_dynamic_timeout 13.6s
tests/explorer/scheduler_test.py::SchedulerTest::test_get_results 20.8s
tests/explorer/scheduler_test.py::SchedulerTest::test_metric_calculation_with_non_repeatable_workflow_0 5.4s
tests/explorer/scheduler_test.py::SchedulerTest::test_metric_calculation_with_non_repeatable_workflow_1 5.5s
tests/explorer/scheduler_test.py::SchedulerTest::test_metric_calculation_with_repeatable_workflow_0 5.3s
tests/explorer/scheduler_test.py::SchedulerTest::test_metric_calculation_with_repeatable_workflow_1 5.3s
tests/explorer/scheduler_test.py::SchedulerTest::test_multi_step_execution 5.9s
tests/explorer/scheduler_test.py::SchedulerTest::test_non_repeatable_workflow 5.5s
tests/explorer/scheduler_test.py::SchedulerTest::test_over_rollout_min_wait 9.3s
tests/explorer/scheduler_test.py::SchedulerTest::test_scheduler_all_methods 15.3s
tests/explorer/scheduler_test.py::SchedulerTest::test_scheduler_restart_after_stop 9.9s
tests/explorer/scheduler_test.py::SchedulerTest::test_split_tasks 9.2s
tests/explorer/scheduler_test.py::SchedulerTest::test_stepwise_experience_eid 25.5s
tests/explorer/scheduler_test.py::SchedulerTest::test_wait_all 8.5s
tests/explorer/scheduler_test.py::SchedulerTest::test_wait_all_timeout_with_multi_batch 14.1s
tests/explorer/scheduler_test.py::TestRunnerStateCollection::test_runner_state_collection 10.4s
tests/explorer/step_wise_workflow_test.py::WorkflowTest::test_reward_propagation_workflow_0 1ms
tests/explorer/step_wise_workflow_test.py::WorkflowTest::test_reward_propagation_workflow_1 602ms
tests/explorer/step_wise_workflow_test.py::WorkflowTest::test_step_wise_reward_workflow_0 1ms
tests/explorer/step_wise_workflow_test.py::WorkflowTest::test_step_wise_reward_workflow_1 1.0s
tests/explorer/step_wise_workflow_test.py::WorkflowTest::test_workflows_raise_error 1ms
tests/explorer/step_wise_workflow_test.py::WorkflowTest::test_workflows_stop_at_max_env_steps 1.0s
tests/explorer/workflow_test.py::WorkflowTest::test_gsm8k_workflow 27ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_boxed_workflow 17ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_complex_workflow 133ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_eval_workflow 3ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_fraction_workflow 11ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_workflow 8ms
tests/explorer/workflow_test.py::WorkflowTest::test_rm_gallery_workflow 138ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_repeatable_0 1ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_repeatable_1 100ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_resettable_0 1ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_resettable_1 202ms
tests/explorer/workflow_test.py::MultiTurnWorkflowTest_0::test_multi_turn_workflow 21.2s
tests/explorer/workflow_test.py::MultiTurnWorkflowTest_1::test_multi_turn_workflow 20.4s
tests/explorer/workflow_test.py::TestWorkflowStateRecording::test_workflow_state_recording 4.0s
tests/explorer/workflow_test.py::TestAgentScopeWorkflowAdapter::test_adapter_v0 712ms
tests/explorer/workflow_test.py::TestAgentScopeWorkflowAdapter::test_adapter_v1 ⏭️ 1ms
tests/explorer/workflow_test.py::TestWorkflowRunner::test_workflow_runner 136ms
tests/explorer/workflow_test.py::TestWorkflowRunner::test_workflow_runner_get_state 8.0s
tests/explorer/workflow_test.py::TestWorkflowRunner::test_workflow_with_openai 22.6s

Github Test Reporter by CTRF 💚

raise NotImplementedError

def update(self, indices: List[int], values: List[float]) -> None:
def feedback(self, indices: List[int], values: List[float]) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document should be update accordingly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants