Deniz/dc agent updates 1215#13
Conversation
There was a problem hiding this comment.
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)
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)
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
- 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)
This PR
In tandem with PR harbor-framework/harbor#232