Do safe preprocess optimizations. #4444
Conversation
jonathanmetzman
commented
Nov 26, 2024
jonathanmetzman
commented
- Don't unnecessarily list features about blobs other than names
- Memoize leak blacklist stuff.
- Properly skip cleanup.
1. Don't unnecessarily list features about blobs other than names 2. Memoize leak blacklist stuff. 3. Properly skip cleanup.
|
This has some of the changes from #4431 while I wait for a review. |
| @@ -80,6 +80,8 @@ class AlreadyRunningError(Error): | |||
| def cleanup_task_state(): | |||
| """Cleans state before and after a task is executed.""" | |||
| # Cleanup stale processes. | |||
| if not environment.is_tworker(): | |||
There was a problem hiding this comment.
How bad is it if this codepath gets hit by the uworker? Why not an assertion?
There was a problem hiding this comment.
I expect the codepath to be hit by both uworker and tworker (is that what you mean).
I just don't want to do the cleanup in a twoker. It's pretty slow and it's doing things a tworker doesn't need like killing processes and removing tmpdirs. tworker doesn't really need this because it's not actually running or downloading anything.
| @@ -468,7 +470,6 @@ def process_command_impl(task_name, task_argument, job_name, high_end, | |||
| return run_command(task_name, task_argument, job_name, uworker_env) | |||
| finally: | |||
| # Final clean up. | |||
| if not environment.is_tworker(): | |||
| cleanup_task_state() | |||
| cleanup_task_state() | |||
There was a problem hiding this comment.
I assume this got removed from L83, since you make sure that this code is not reached by the uworker? Is there anything other than the uworker that you want to avoid reaching this code path? Or is there any other mechanism in place that would avoid the old untrusted worker architecture to get here?
There was a problem hiding this comment.
Sorry do you mean a different PR?
There was a problem hiding this comment.
I find the question kind of confusing, but I'm no longer relying on that old if statement, the new one is better because the function is called from more than one place.
This doesn't really have anything to do with the untrusted arch.
