[BC] keep_temps update
#411
Conversation
inlining for size.
or neither of them is set. Further, passes keep_temps to ```compilation_runner.get_workdir_context``` and ensures that the flag and argument are not set at the same time.
| @@ -80,14 +80,23 @@ def __exit__(self, exc, value, tb): | |||
| pass | |||
|
|
|||
|
|
|||
| def get_workdir_context(): | |||
| def get_workdir_context(keep_temps: Optional[str] = None): | |||
There was a problem hiding this comment.
nit: s/keep_temps/explicit_temps_dir. Otherwise it sounds like a boolean (yes, documentation explains, but why call the object bicycle when it's a car lol)
| """ | ||
| if keep_temps and _KEEP_TEMPS.value: |
There was a problem hiding this comment.
I see... _KEEP_TEMPS... maybe we can refactor this thing's name after. But let's name the API clearly.
Or you can rename both in one swoop after, up to you.
| @@ -46,6 +46,13 @@ | |||
| from compiler_opt.distributed import buffered_scheduler | |||
| from compiler_opt.distributed.local import local_worker_manager | |||
|
|
|||
| _BASE_PATH = flags.DEFINE_string( | |||
There was a problem hiding this comment.
maybe _PERSISTENT_OBJECTS_PATH or something - "base" is very generic.
btw, could we "hitch" this path to be a subdirectory of where we save the model?
There was a problem hiding this comment.
Not completely sure how we can do this since the code that trains the model is completely separate from this. We could potentially replace this with a sub-directory of where we save the collected trajectories.
| @@ -918,6 +927,10 @@ def gen_trajectories( | |||
| worker_manager_class: A pool of workers hosted on the local machines, each | |||
| in its own process. | |||
| """ | |||
| if (None in (_BASE_PATH.value, FLAGS.keep_temps) and | |||
| not all(el is None for el in (_BASE_PATH.value, FLAGS.keep_temps))): | |||
| raise ValueError(('Both flags keep_temps={FLAGS.keep_temps} and' | |||
There was a problem hiding this comment.
I think the if statement could probably be simpler if written without all the Python syntactic sugar?
if (_BASE_PATH.value is None or FLAGS.keep_temps is None) and (_BASE_PATH.value is not None and FLAGS.keep_temps is not None):
passThey're both understandable, but I think this is a quite a bit more readable.
| if (None in (_BASE_PATH.value, FLAGS.keep_temps) and | ||
| not all(el is None for el in (_BASE_PATH.value, FLAGS.keep_temps))): | ||
| raise ValueError(('Both flags keep_temps={FLAGS.keep_temps} and' | ||
| 'base_path={_BASE_PATH.value} should be set or be None')) |
There was a problem hiding this comment.
Is there a reason that both of these have to be set? I think it would make sense to turning on keeping temps if either of the flags is set rather than requiring both.
(Sorry if I missed some conversation about this on the API design side).
There was a problem hiding this comment.
I think keep_temps can be set independently of base_path but if base path is set keep_temps should be set, so I will do this check instead.
explicit_temps_dir is not set and persistent_objects_path is set.
Rename
base_pathtopersistent_objects_pathandkeep_tempstoexplicit_temps_dir.Makes sure that if
persistent_objects_pathis set thenexplicit_temps_diris also setor neither of them is set. Further, passes
explicit_temps_dirtocompilation_runner.get_workdir_contextand ensures that the flagand argument are not set at the same time.