-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Closed
Labels
refactoringNo change to functionalityNo change to functionality
Description
The Collector tracks the self.data
field that is a RolloutBatch
. However, rather confusingly, this value of this field is recreated and updated at every call to collect
. This leads to a lot of methods mutating the state, and a less comprehensible and debuggable code.
There is no need to bind this to the lifetime of the Collector, instead it should be created and used as a var in collect
.
Moreover, there is the possibility to pass preprocess_fn
, which
- doesn't have a clear interfaces
- doesn't have a clear purpose
- Is only used in a test and nowhere else
It seems like the original purpose of that was to enable specific logging needs, but these needs can be met by different means. - Significally complicates the Collector
The preprocess_fn
in Collector
should be removed
This issue blocks #1046 since we need to reduce the complexity of the Collector before doing anything further with it
@bordeauxred is working on this issue
Metadata
Metadata
Assignees
Labels
refactoringNo change to functionalityNo change to functionality
Type
Projects
Status
Done