Repository navigation
Conversation
…to_dir
Check `f"name:{searcher_name}" in search_alg_state` when saving nested search wrappers in `SearchGenerator.save_to_dir`, matching the `"name:"` prefix used when storing keys in `save_to_dir` and reading them in `restore_from_dir`. Previously, the unprefixed check always evaluated to `False`, suppressing the duplicate wrapper warning and overwriting the outer wrapper's saved state.
Signed-off-by: Parth Vadera <parthvadera@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in SearchGenerator.save_to_dir where duplicate searcher states were not correctly identified due to a mismatch between the checked key and the saved key. This is resolved by introducing a consistent state_key variable. Additionally, a unit test is added to verify the save and restore behavior of nested searcher wrappers. The review feedback suggests using an f-string for the warning logger to maintain consistency and adding an assertion to the new test to verify that the inner wrapper's state is not incorrectly overwritten.
| logger.warning( | ||
| "There was a duplicate when saving {}. " | ||
| "Restore may not work properly.".format(searcher_name) | ||
| ) |
There was a problem hiding this comment.
Since an f-string is already used on line 175 to construct state_key, we should also use an f-string here for the warning message to maintain consistency and improve readability.
| logger.warning( | |
| "There was a duplicate when saving {}. " | |
| "Restore may not work properly.".format(searcher_name) | |
| ) | |
| logger.warning( | |
| f"There was a duplicate when saving {searcher_name}. " | |
| "Restore may not work properly." | |
| ) |
| ) | ||
| restored_alg = SearchGenerator(restored_outer) | ||
| restored_alg.restore_from_dir(str(tmp_path)) | ||
| assert restored_outer.repeat == 2 |
There was a problem hiding this comment.
To make this test more robust and explicitly verify that the inner repeater's state was not incorrectly overwritten or restored with the outer repeater's state, we should also assert that the inner repeater's repeat value remains unchanged (i.e., 1).
| assert restored_outer.repeat == 2 | |
| assert restored_outer.repeat == 2 | |
| assert restored_outer.searcher.repeat == 1 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit d16e500. Configure here.
ray-gardener
Bot
added
tune
Use an f-string for the duplicate wrapper warning in SearchGenerator.save_to_dir, add the propagate_logs fixture to test_search_generator_duplicate_wrapper_save_restore, and assert that the inner repeater state is preserved on restore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Parth Vadera <parthvadera@gmail.com>
Why are these changes needed?
In
SearchGenerator.save_to_dir(python/ray/tune/search/search_generator.py), wrapper searcher state is stored insearch_alg_stateunder the"name:"-prefixed key (search_alg_state["name:" + searcher_name] = searcher.get_state()) and restored inrestore_from_dirusingsearcher_name = "name:" + type(searcher).__name__.However, the duplicate check in
save_to_dirpreviously checkedif searcher_name in search_alg_state:without the"name:"prefix. Because keys insearch_alg_stateare prefixed with"name:", this check always evaluated toFalse:"There was a duplicate when saving ... Restore may not work properly.") was never logged.search_alg_state["name:" + searcher_name], causingrestore_from_dirto restore the inner wrapper's state into the outer wrapper.This PR checks
state_key = f"name:{searcher_name}"againstsearch_alg_stateinsave_to_dirso duplicates are properly detected and the outer wrapper state is preserved.Acknowledgments
Thanks to the Ray Tune maintainers and contributors who designed
SearchGeneratorand its wrapper state serialization (Repeater,ConcurrencyLimiter), and to @gemini-code-assist[bot] and @cursor[bot] for suggesting the f-string warning,propagate_logsfixture, and inner-wrapper state assertion.Checks
by: Parth Vadera <parthvadera@gmail.com>)test_search_generator_duplicate_wrapper_save_restoreinpython/ray/tune/tests/test_searcher_utils.py.