-
Notifications
You must be signed in to change notification settings - Fork 92
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
[BC] Refactor _KEEP_TEMPS
for reusability
#376
Conversation
given by combine_tfa_policies_lib.get_input_signature() and action spec given by combine_tfa_policies_lib.get_action_spec() The combiner policy uses a new timestep spec feature "model_selector" to select the requested policy at the current state. The feature is computed as a md5 hash from the respective policies names.
…iler-opt into policy_combiner
TimeStep. The patch also gives the option to keep the temporary working_dir by setting keep_temps in compilation_runner.py to a directory where all temporary working_dirs will be saved.
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.
looks good, just 2 comments, feel free to land after addressing them.
@@ -80,6 +80,18 @@ def __exit__(self, exc, value, tb): | |||
pass | |||
|
|||
|
|||
def get_directory_context(): |
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.
I'd rename this to be more specific, how about get_workdir_context
(otherwise... lots of directories, so which one is this :) )
compiler_opt/rl/env_test.py
Outdated
self.assertEqual(os.path.exists(working_dir), False) | ||
|
||
with flagsaver.flagsaver( | ||
(env.compilation_runner._KEEP_TEMPS, '/tmp')): # pylint: disable=protected-access |
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.
can you use instead of /tmp
self.create_tempdir()
?
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.
Can you check if this looks good now?
Can you rename also the title of the change to e.g. "factor _KEEP_TEMP for reusability" or something in that vein |
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.
One minor nit, otherwise LGTM.
"""Return a context which manages how the temperory directories are handled. | ||
|
||
When the flag keep_temps is specified temporary directories are stored in | ||
keep_temps.""" |
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.
Nit: The """
string terminator should be on a separate line.
_KEEP_TEMPS
for reusability
Patch that adds the option to keep temporary working_dir in env.py and adds working_dir as part of TimeStep in env.py