Skip to content

Conversation

@dzorlu
Copy link
Collaborator

@dzorlu dzorlu commented Dec 16, 2025

This PR

  • fixes issues with the retries
  • passes a new flag to store all messages

In tandem with PR laude-institute/harbor#232

Copy link

@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 correctly fixes an issue with trial retries by re-instantiating the Trial object within the retry loop. It also adds a new store_all_messages flag. My review includes a suggestion to make this new flag configurable for improved maintainability and a minor style correction to remove extraneous whitespace introduced in the changes.

I am having trouble creating individual review comments. Click here to see my feedback.

skyrl-train/examples/terminal_bench/terminal_bench_generator.py (152)

medium

Hardcoding store_all_messages to True reduces the flexibility of this generator. It would be better to make this configurable by reading the value from terminal_bench_cfg in the __init__ method, similar to how other parameters are handled. This would allow for easier modification of this behavior without changing the code.

skyrl-train/examples/terminal_bench/terminal_bench_generator.py (169-170)

medium

This change introduces a line containing only whitespace and results in two consecutive blank lines before a comment. This is extraneous whitespace and violates PEP 8 style guidelines, which recommend using blank lines sparingly to separate logical sections. Please remove the whitespace-only line and one of the blank lines to improve readability.

References
  1. PEP 8, the style guide for Python code, advises against extraneous whitespace, including lines with only whitespace and excessive blank lines. Blank lines should be used sparingly to indicate logical sections. (link)

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.

1 participant