[analyze][rearchitecture] Reduce delays on untrusted parts of analyze #3700
Conversation
… task Instead of preprocessing and then putting on the queue for scheduling, schedule right away. This eliminates up to 1 minute of time spent pulling tasks from the queue and can improve execution speed on batch by: 1. Causing batch to schedule the job earlier because the job requires less CPUs than jobs with many tasks. These CPUs may not be available in times of high-usage. 2. Allowing us to set a higher priority for the job.
| """Represents an untrusted task. Executes preprocess on this machine, then | ||
| queues up the task for scheduling on batch by putting it in the utask_main | ||
| queue. Then executes utask_main on an untrusted machine, and postprocess on | ||
| another trusted machine if opted-in. Otherwise executes locally.""" |
There was a problem hiding this comment.
What executes locally vs remotely has gotten a bit confused with the comment change. It would be good to clarify that either:
- utask_main is scheduled on cloud batch, then postprocess executes on another trusted bot
- or everything executes locally immediately
| logs.log('Requesting remote execution for utask.') | ||
| self.request_remote_execution(command, download_url, job_type) | ||
|
|
||
| def request_remote_execution(self, command, download_url, job_type): |
There was a problem hiding this comment.
This function seems redundant?
Edit: ah no, I see now.
| tasks.add_utask_main(command, download_url, job_type) | ||
|
|
||
| def preprocess(self, task_argument, job_type, uworker_env): | ||
| logs.log('Preprocessing utask.') |
There was a problem hiding this comment.
Likely useful to include task_argument in the log, at least.
| @@ -136,6 +140,20 @@ def preprocess(self, task_argument, job_type, uworker_env): | |||
| return download_url | |||
|
|
|||
|
|
|||
| class LowLatencyUTask(UTask): | |||
There was a problem hiding this comment.
Rather than inheritance, I suggest adding a boolean member to UTask that controls low-latency behavior, and behaving differently based on its value in UTask.request_temote_execution().
Edit: I see, COMMAND_TYPES requires a mapping from task name to executor class, which kind of forces us to use inheritance over composition. Then consider:
class AbstractUTask(abc.ABC):
... stuff ...
@abstractmethod
def request_remote_execution(...):
...
class UTask(AbstractUTask):
def request_remote_execution(...):
... schedule on the queue ...
class LowLatencyUTask(AbstractUTask):
def request_remote_execution(...):
... schedule on batch ...At least, it makes the extension point obvious.
| @@ -57,6 +57,8 @@ | |||
| 'machine_type', | |||
| ]) | |||
|
|
|||
| HIGH_PRIORITY = 50 | |||
There was a problem hiding this comment.
This could use a comment explaining the choice of the value.
Co-authored-by: Titouan Rigoudy <titouan@chromium.org>
jonathanmetzman
force-pushed
the
master
branch
2 times, most recently
from
e7e91a0
to
22e1108
Compare
Instead of preprocessing and then putting on the queue for scheduling,
schedule right away. This eliminates up to 1 minute of time spent
pulling tasks from the queue and can improve execution speed on batch
by:
requires less CPUs than jobs with many tasks. These CPUs may not
be available in times of high-usage.