-
Notifications
You must be signed in to change notification settings - Fork 3k
Revert "[recipe] feat: Add sleep/wakeup mode for gen rm vllm service and add tqdm showing process" #2813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…and add…" This reverts commit 76298ad.
There was a problem hiding this 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 reverts a previous feature related to vLLM service sleep/wakeup modes, presumably to fix CI issues. The changes correctly remove the feature's implementation and documentation. My review focuses on the resulting code within the diff. I've identified an opportunity to improve the conciseness and maintainability of the task submission logic in reward_function.py by using a more idiomatic Python construct. The provided suggestion will make the code cleaner without altering its behavior.
| for data_source, solution_str, ground_truth, extra_info in zip( | ||
| data_sources, solution_strs, ground_truths, extra_infos, strict=True | ||
| ): | ||
| future = executor.submit(compute_score, data_source, solution_str, ground_truth, extra_info, index) | ||
| time.sleep(0.001 * random.random()) | ||
| future = executor.submit(compute_score, data_source, solution_str, ground_truth, extra_info) | ||
| futures.append(future) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The for loop used to submit tasks to the ThreadPoolExecutor is unnecessarily verbose. This can be refactored into a more concise and idiomatic list comprehension.
This change improves readability and maintainability by replacing a multi-line loop with a single, expressive statement, which is a common pattern in modern Python for this type of task.
| for data_source, solution_str, ground_truth, extra_info in zip( | |
| data_sources, solution_strs, ground_truths, extra_infos, strict=True | |
| ): | |
| future = executor.submit(compute_score, data_source, solution_str, ground_truth, extra_info, index) | |
| time.sleep(0.001 * random.random()) | |
| future = executor.submit(compute_score, data_source, solution_str, ground_truth, extra_info) | |
| futures.append(future) | |
| futures = [ | |
| executor.submit(compute_score, data_source, solution_str, ground_truth, extra_info) | |
| for data_source, solution_str, ground_truth, extra_info in zip( | |
| data_sources, solution_strs, ground_truths, extra_infos, strict=True | |
| ) | |
| ] |
…and add tqdm showing process" (volcengine#2813) Reverts volcengine#2739 For volcengine#2794 to solve all CI faults.
…and add tqdm showing process" (volcengine#2813) Reverts volcengine#2739 For volcengine#2794 to solve all CI faults.
…and add tqdm showing process" (volcengine#2813) Reverts volcengine#2739 For volcengine#2794 to solve all CI faults.
…and add tqdm showing process" (volcengine#2813) Reverts volcengine#2739 For volcengine#2794 to solve all CI faults.
…and add tqdm showing process" (volcengine#2813) Reverts volcengine#2739 For volcengine#2794 to solve all CI faults.
Reverts #2739
For #2794 to solve all CI faults.