Skip to content

Conversation

@chenyushuo
Copy link
Collaborator

Description

As the title says.

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

@chenyushuo
Copy link
Collaborator Author

/unittest-module-buffer

@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 addresses a critical bug within the TaskScheduler component that caused incorrect termination logic when total_steps was specified. The core change modifies the condition for stopping task iteration, ensuring that the system respects the total_steps configuration and only falls back to total_epochs if steps are not defined. This prevents premature halts and allows the scheduler to operate as intended under step-based execution. Complementary changes include the addition of new, more robust test cases to validate this corrected behavior and a more flexible test configuration mechanism.

Highlights

  • Bug Fix: total_steps handling: Corrected a bug in the TaskScheduler's read_async method where the scheduler would prematurely stop based on total_epochs even when total_steps was configured. The fix ensures that total_epochs is only considered as a termination condition if total_steps is not explicitly set.
  • Enhanced Test Coverage: Added new parameterized test cases to test_task_scheduler to specifically validate the TaskScheduler's behavior when total_steps is configured, ensuring the bug fix works as expected and preventing regressions.
  • Refactored Test Setup: The test_task_scheduler function has been refactored to dynamically accept buffer configuration parameters (like batch_size, total_steps, total_epochs) through buffer_config_kwargs, making the tests more flexible and easier to extend for various scenarios.
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.

@chenyushuo
Copy link
Collaborator Author

/unittest-module-explorer

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 addresses a bug related to the total_steps parameter in the TasksetScheduler. The fix in trinity/buffer/task_scheduler.py correctly prevents premature termination based on total_epochs when total_steps is specified. The accompanying changes in tests/buffer/task_scheduler_test.py are commendable, adding a specific test case for the total_steps functionality and refactoring existing tests to be more flexible and parameterized. Overall, this is a high-quality contribution that both fixes a bug and improves the test suite.

@github-actions
Copy link

Summary

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

Tests

Test Name Status Flaky Duration
tests/buffer/experience_pipeline_test.py::TestExperiencePipeline::test_experience_pipeline 28.3s
tests/buffer/experience_pipeline_test.py::TestExperiencePipeline::test_pass_rate_calculation 15.8s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_experience_buffer 4.1s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_storage_0_sft 6.0s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_storage_1_dpo 6.3s
tests/buffer/file_test.py::TestFileBuffer::test_file_reader 454ms
tests/buffer/file_test.py::TestFileBuffer::test_file_writer 4.2s
tests/buffer/formatter_test.py::TestFormatter::test_dpo_messages_formatter 536ms
tests/buffer/formatter_test.py::TestFormatter::test_dpo_plaintext_formatter 476ms
tests/buffer/formatter_test.py::TestFormatter::test_multi_modal_sft_formatter 805ms
tests/buffer/formatter_test.py::TestFormatter::test_sft_messages_formatter 994ms
tests/buffer/formatter_test.py::TestFormatter::test_sft_plaintext_formatter 762ms
tests/buffer/formatter_test.py::TestFormatter::test_task_formatter 223ms
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_buffer_reuse 8.9s
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_capacity 4.7s
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_reuse_count_control 7.0s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_0_queue 5.9s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_1_priority_queue 5.8s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_capacity 6.4s
tests/buffer/reward_shaping_mapper_test.py::TestRewardShapingMapper::test_basic_usage 6ms
tests/buffer/sql_test.py::TestSQLBuffer::test_sql_buffer_read_write 4.5s
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_0 87ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_1 107ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_2 106ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_3 105ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_4 110ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_5 127ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_0 68ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_1 3.8s
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_2 47ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_3 4.1s
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_4 48ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_5 4.6s

Github Test Reporter by CTRF 💚

@github-actions
Copy link

Summary

Tests 📝 Passed ✅ Failed ❌ Skipped ⏭️ Other ❓ Flaky 🍂 Duration ⏱️
36 35 0 1 0 0 11m 56s

Skipped

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

Tests

Test Name Status Flaky Duration
tests/explorer/explorer_test.py::TestExplorerCountdownEval::test_explorer 1m 11s
tests/explorer/explorer_test.py::TestExplorerGSM8KRULERNoEval::test_explorer 1m 42s
tests/explorer/explorer_test.py::TestExplorerGSM8k::test_explorer 3m 38s
tests/explorer/explorer_test.py::ServeTest::test_serve 1m 25s
tests/explorer/scheduler_test.py::SchedulerTest::test_async_workflow 12.3s
tests/explorer/scheduler_test.py::SchedulerTest::test_concurrent_operations 12.2s
tests/explorer/scheduler_test.py::SchedulerTest::test_get_results 30.5s
tests/explorer/scheduler_test.py::SchedulerTest::test_multi_step_execution 12.4s
tests/explorer/scheduler_test.py::SchedulerTest::test_non_repeatable_workflow 12.4s
tests/explorer/scheduler_test.py::SchedulerTest::test_scheduler_all_methods 22.6s
tests/explorer/scheduler_test.py::SchedulerTest::test_scheduler_restart_after_stop 23.9s
tests/explorer/scheduler_test.py::SchedulerTest::test_split_tasks 15.7s
tests/explorer/scheduler_test.py::SchedulerTest::test_stepwise_experience_eid 12.3s
tests/explorer/scheduler_test.py::SchedulerTest::test_wait_all 15.5s
tests/explorer/scheduler_test.py::SchedulerTest::test_wait_all_timeout_with_multi_batch 21.2s
tests/explorer/step_wise_workflow_test.py::WorkflowTest::test_reward_propagation_workflow_0 2ms
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 35ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_boxed_workflow 24ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_complex_workflow 179ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_eval_workflow 4ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_fraction_workflow 14ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_workflow 8ms
tests/explorer/workflow_test.py::WorkflowTest::test_rm_gallery_workflow 87ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_repeatable_0 1ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_repeatable_1 101ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_resettable_0 1ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_resettable_1 201ms
tests/explorer/workflow_test.py::MultiTurnWorkflowTest_0::test_multi_turn_workflow 16.4s
tests/explorer/workflow_test.py::MultiTurnWorkflowTest_1::test_multi_turn_workflow 14.7s
tests/explorer/workflow_test.py::TestAgentScopeWorkflowAdapter::test_adapter ⏭️ 1ms
tests/explorer/workflow_test.py::TestWorkflowRunner::test_workflow_runner 294ms

Github Test Reporter by CTRF 💚

@pan-x-c
Copy link
Collaborator

pan-x-c commented Nov 14, 2025

/unittest-module-buffer

@github-actions
Copy link

Summary

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

Tests

Test Name Status Flaky Duration
tests/buffer/experience_pipeline_test.py::TestExperiencePipeline::test_experience_pipeline 28.6s
tests/buffer/experience_pipeline_test.py::TestExperiencePipeline::test_pass_rate_calculation 15.4s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_experience_buffer 4.0s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_storage_0_sft 6.0s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_storage_1_dpo 6.4s
tests/buffer/file_test.py::TestFileBuffer::test_file_reader 154ms
tests/buffer/file_test.py::TestFileBuffer::test_file_writer 4.2s
tests/buffer/formatter_test.py::TestFormatter::test_dpo_messages_formatter 533ms
tests/buffer/formatter_test.py::TestFormatter::test_dpo_plaintext_formatter 471ms
tests/buffer/formatter_test.py::TestFormatter::test_multi_modal_sft_formatter 808ms
tests/buffer/formatter_test.py::TestFormatter::test_sft_messages_formatter 987ms
tests/buffer/formatter_test.py::TestFormatter::test_sft_plaintext_formatter 725ms
tests/buffer/formatter_test.py::TestFormatter::test_task_formatter 221ms
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_buffer_reuse 8.9s
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_capacity 4.7s
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_reuse_count_control 6.9s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_0_queue 5.9s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_1_priority_queue 5.9s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_capacity 6.4s
tests/buffer/reward_shaping_mapper_test.py::TestRewardShapingMapper::test_basic_usage 6ms
tests/buffer/sql_test.py::TestSQLBuffer::test_sql_buffer_read_write 4.5s
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_0 88ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_1 107ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_2 106ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_3 106ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_4 111ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_5 128ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_0 68ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_1 3.9s
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_2 48ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_3 3.8s
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_4 48ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_5 4.6s

Github Test Reporter by CTRF 💚

@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 ⏱️
34 34 0 0 0 0 2m 8s

Tests

Test Name Status Flaky Duration
tests/buffer/experience_pipeline_test.py::TestExperiencePipeline::test_experience_pipeline 27.7s
tests/buffer/experience_pipeline_test.py::TestExperiencePipeline::test_pass_rate_calculation 15.3s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_experience_buffer 4.1s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_storage_0_sft 6.0s
tests/buffer/experience_storage_test.py::ExperienceStorageTest::test_sql_storage_1_dpo 6.5s
tests/buffer/file_test.py::TestFileBuffer::test_file_reader 454ms
tests/buffer/file_test.py::TestFileBuffer::test_file_writer 4.4s
tests/buffer/formatter_test.py::TestFormatter::test_dpo_messages_formatter 522ms
tests/buffer/formatter_test.py::TestFormatter::test_dpo_plaintext_formatter 459ms
tests/buffer/formatter_test.py::TestFormatter::test_multi_modal_sft_formatter 783ms
tests/buffer/formatter_test.py::TestFormatter::test_sft_messages_formatter 959ms
tests/buffer/formatter_test.py::TestFormatter::test_sft_plaintext_formatter 711ms
tests/buffer/formatter_test.py::TestFormatter::test_task_formatter 225ms
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_buffer_reuse 8.9s
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_capacity 4.9s
tests/buffer/queue_test.py::TestQueueBuffer::test_priority_queue_reuse_count_control 6.6s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_0_queue 5.6s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_1_priority_queue 5.7s
tests/buffer/queue_test.py::TestQueueBuffer::test_queue_buffer_capacity 6.2s
tests/buffer/reward_shaping_mapper_test.py::TestRewardShapingMapper::test_basic_usage 5ms
tests/buffer/sql_test.py::TestSQLBuffer::test_sql_buffer_read_write 4.5s
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_0 97ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_1 76ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_2 113ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_3 106ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_4 107ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_5 111ms
tests/buffer/task_scheduler_test.py::TestTaskScheduler::test_task_scheduler_6 128ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_0 69ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_1 3.9s
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_2 47ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_3 4.1s
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_4 48ms
tests/buffer/task_storage_test.py::TaskStorageTest::test_read_task_5 4.7s

Github Test Reporter by CTRF 💚

@github-actions
Copy link

Summary

Tests 📝 Passed ✅ Failed ❌ Skipped ⏭️ Other ❓ Flaky 🍂 Duration ⏱️
36 35 0 1 0 0 11m 58s

Skipped

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

Tests

Test Name Status Flaky Duration
tests/explorer/explorer_test.py::TestExplorerCountdownEval::test_explorer 1m 14s
tests/explorer/explorer_test.py::TestExplorerGSM8KRULERNoEval::test_explorer 1m 45s
tests/explorer/explorer_test.py::TestExplorerGSM8k::test_explorer 3m 39s
tests/explorer/explorer_test.py::ServeTest::test_serve 1m 20s
tests/explorer/scheduler_test.py::SchedulerTest::test_async_workflow 12.5s
tests/explorer/scheduler_test.py::SchedulerTest::test_concurrent_operations 12.0s
tests/explorer/scheduler_test.py::SchedulerTest::test_get_results 30.8s
tests/explorer/scheduler_test.py::SchedulerTest::test_multi_step_execution 12.5s
tests/explorer/scheduler_test.py::SchedulerTest::test_non_repeatable_workflow 12.3s
tests/explorer/scheduler_test.py::SchedulerTest::test_scheduler_all_methods 22.2s
tests/explorer/scheduler_test.py::SchedulerTest::test_scheduler_restart_after_stop 23.9s
tests/explorer/scheduler_test.py::SchedulerTest::test_split_tasks 15.7s
tests/explorer/scheduler_test.py::SchedulerTest::test_stepwise_experience_eid 12.7s
tests/explorer/scheduler_test.py::SchedulerTest::test_wait_all 15.1s
tests/explorer/scheduler_test.py::SchedulerTest::test_wait_all_timeout_with_multi_batch 21.2s
tests/explorer/step_wise_workflow_test.py::WorkflowTest::test_reward_propagation_workflow_0 2ms
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 34ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_boxed_workflow 24ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_complex_workflow 175ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_eval_workflow 3ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_fraction_workflow 13ms
tests/explorer/workflow_test.py::WorkflowTest::test_math_workflow 8ms
tests/explorer/workflow_test.py::WorkflowTest::test_rm_gallery_workflow 86ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_repeatable_0 1ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_repeatable_1 101ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_resettable_0 1ms
tests/explorer/workflow_test.py::WorkflowTest::test_workflow_resettable_1 201ms
tests/explorer/workflow_test.py::MultiTurnWorkflowTest_0::test_multi_turn_workflow 15.5s
tests/explorer/workflow_test.py::MultiTurnWorkflowTest_1::test_multi_turn_workflow 14.4s
tests/explorer/workflow_test.py::TestAgentScopeWorkflowAdapter::test_adapter ⏭️ 1ms
tests/explorer/workflow_test.py::TestWorkflowRunner::test_workflow_runner 293ms

Github Test Reporter by CTRF 💚

@pan-x-c pan-x-c merged commit ba33438 into modelscope:main Nov 14, 2025
1 check passed
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