rwth-i6 / sisyphus

A Workflow Manager in Python
Mozilla Public License 2.0
45 stars 25 forks source link

Add threads for alias and output updates on startup #214

Closed Atticus1806 closed 5 days ago

Atticus1806 commented 1 month ago

Fix #213

albertz commented 1 month ago

How thread-safe is Sisyphus? There is no potential issue with that?

Atticus1806 commented 1 month ago

Since this is just alias and output updates I dont think there is a problem. These ops should be executable any time

albertz commented 1 month ago

I'm more thinking about all the internal caching mechanisms of jobs, outputs, paths, the graph, etc. All of that must be thread-safe. (But it might be already, as we also use multi-threading elsewhere. I don't recall the details now.)

Atticus1806 commented 1 month ago

While my impression is they are, I am passing the question to the experts since I dont know for sure.

Atticus1806 commented 2 weeks ago

I updated to use the thread pool.

It will be called again inside the manager loop anyway.

While that is true, its called with other parameters right? So a "full" update is only done at the beginning. I dont mind removing it here and then saying "user needs to call update for a full update", but this should be discussed. Or do we add a flag Skip_Output_update_on_startup or smth.? If we want to keep it but thread it: any suggestions on how to check if this actually has no unwanted behavior? Maybe we can call it only for non active outputs or smth?

critias commented 2 weeks ago

Good point, a simple solution might be to just add a semaphore at the start of check_output to ensure that it's not run twice in parallel.

Atticus1806 commented 2 weeks ago

I am currently testing the semaphore version, but what I am wondering about is this line: https://github.com/rwth-i6/sisyphus/blob/d256f43b69c0dd668b52822ec0016cb8479e5efd/sisyphus/manager.py#L485 This looks quite redundant too me, since it is called in all relevant paths later aswell. Maybe this can be removed here?

albertz commented 5 days ago

What is the status here?

Atticus1806 commented 5 days ago

@critias do you think this is safe now, as it is?

critias commented 5 days ago

I don't see any problem anymore (beside changing the Semaphore into a Look). Thank you!