Add es trainer setup and loop #288
Conversation
salaast
force-pushed
the
es_trainer_files
branch
from
3f8bb83
to
17299ae
Compare
compiler_opt/es/blackbox_learner.py
Outdated
| sampler: corpus.Corpus, | ||
| tf_policy_path: str, | ||
| output_dir: str, | ||
| policy_saver_fn: Callable[..., Any], |
compiler_opt/es/blackbox_learner.py
Outdated
| config: configuration for blackbox optimization. | ||
| stubs: grpc stubs to inlining/regalloc servers | ||
| initial_step: the initial step for learning. | ||
| deadline: the deadline for requests to the inlining server. |
There was a problem hiding this comment.
is this in seconds? either rename deadline_seconds or add it in the docstring.
compiler_opt/es/blackbox_learner.py
Outdated
| perturbations = [] | ||
| for _ in range(self._config.total_num_perturbations): | ||
| perturbations.append( | ||
| np.random.normal(size=(len(self._model_weights))) * |
There was a problem hiding this comment.
in general if you wanted to write tests for this, you would pass a seed argument or a np.random.Generator object that was created with a seed; and use that to generate random numbers.
There was a problem hiding this comment.
I do not think I understand what you mean because it does not seem like _get_perturbations or np.random.normal accept a seed or generator argument
There was a problem hiding this comment.
Generator object has a method called normal()
compiler_opt/es/blackbox_learner.py
Outdated
| rewards = [None] * len(results) | ||
|
|
||
| for i in range(len(results)): | ||
| if not results[i].exception(): |
There was a problem hiding this comment.
is there another spot where the exception gets logged, so we don't accidentally drop them?
There was a problem hiding this comment.
I am not sure what you mean by accidentally drop them. My understanding is that if there is an exception then it did not complete the task, so it is certain that there is no reward to be extracted.
There was a problem hiding this comment.
i want to see the error when the task ran, in logs. do we get that now?
There was a problem hiding this comment.
Ah I see. It is not logged at the moment but I can log the exceptions in this section
compiler_opt/es/blackbox_learner.py
Outdated
| policy_obj = policy_saver.Policy.from_filesystem(tfl_dir) | ||
| return policy_obj.policy | ||
|
|
||
| def run_step(self, pool: FixedWorkerPool) -> None: |
There was a problem hiding this comment.
do you need a docstring here? at least mention that this will block until the step is complete.
There was a problem hiding this comment.
Do you mind clarifying what you mean by block? I am not sure what it will be blocking. thanks
There was a problem hiding this comment.
i mean that there's gonna be a bunch of I/O and executions running while this waits for the responses; so it doesn't instantaneously return.
| elements=[corpus.ModuleSpec(name='smth', size=1)], | ||
| additional_flags=(), | ||
| delete_flags=()) | ||
| learner = blackbox_learner.BlackboxLearner( |
There was a problem hiding this comment.
shouldn't at least some of these go into a setUp() method to ensure they stay unmodified across tests?
There was a problem hiding this comment.
Do you mind specifying what would be modified across a test? Are you saying the creation of learner_config, cps, and learner should be moved to a function defined outside of the test class which will be called inside the test class?
There was a problem hiding this comment.
write:
def setUp(self):
super().setUp()
self._cps = ...
self._learner = ...
this will be executed as setup prior to every test run.
|
Since these changes are applicable to blackbox_learner, they have been addressed in pr #286 |
salaast
force-pushed
the
es_trainer_files
branch
from
17299ae
to
60cd41c
Compare
There was a problem hiding this comment.
@ebrevdo - I'd land this as-is - it's overall fine, and it's a leaf component. Would probably want to do something with the mock worker, but can do that after. Main thing is, it has the final parts that enable es training (i.e. an actual main :) )
anything egregious I'm missing?
|
Just nits; nothing blocking. How does this relate to #286 ? |
|
Well; the unit test setUp should maybe get fixed to avoid making it stateful? |
That's the library, this is the driver. |
what unit test? |
|
Landing it so I can try out ES - will refactor a bunch of things shortly, which should address most of the comments here. |
No description provided.