Pass processing step to worker #779
Conversation
The workers factory does not only create dataset-based workers, so it should not be named DatasetsBasedWorkerFactory. Also, to reduce the code, we pass the processing step to the worker at its creation, instead of getting it afterwards.
severo
assigned AndreaFrancis and albertvillanova and unassigned AndreaFrancis and albertvillanova
|
The documentation is not available anymore as the PR was closed or merged. |
|
The CI error is treated here: #778. We will have to wait for it to be fixed before merging this PR. |
There was a problem hiding this comment.
Awesome!!! Thanks!
Definitely it makes much more sense to have the WorkerLoopConfig defined within workers/, where WorkerLoop is defined as well.
Also, as I commented in other PR (#778 (comment)):
Maybe, we could use
BaseWorkerFactoryto define the interface, and renameDatasetBasedWorkerFactorytoWorkerFactory...
I think it is much clearer this way.
And I find the introduction of the processing_graph attribute in the WorkerFactory will help to be more flexible and generalize to more complex processings...
Just some questions below.
| self.app_config = app_config | ||
| self.processing_graph = processing_graph |
There was a problem hiding this comment.
There is one thing I don't understand: the processing_graph continues being part of the app_config? So you are passing it here twice (separate and within the app_config)?
There was a problem hiding this comment.
I mean, if you are trying to decouple them, then ProcessingGraph shouldn't be removed from AppConfig (in workers/datasets_based/src/datasets_based/config.py)?
There was a problem hiding this comment.
oh, you're right.
This PR is a part I extracted from a bigger PR where I'm moving processing_graph outside of the app_config and I forgot to remove it. I'll fix this for now, for the sake of coherence
|
Look @albertvillanova, I merged; I didn't rebase :P. Following your best practice!
|
No description provided.