Move trace_message from arguments to vw object

[jackgerrits]

This is required for the larger task of replacing the arguments object

[JohnLangford]

I may not quite understand the plan.

trace_message was previously in the global datastructure and then was moved to the arguments datastructure when that was created. The core reason for this is that many of the setup() functions need to leave messages and I wanted to shift away from having setup() be dependent on the global data structure. Shifting back, you introduce a spurious dependence on the global datastructure for many reductions. Why?

[rajan-chari]

In the short term, @jackgerrits wanted to remove all non-args logic out of args implementation. Also probably good from a seperation of concerns perspective.

Longer term, as we remove dependence on 'all', there a number of "dependency injection" approaches we can use.

One approach is here:

https://github.com/VowpalWabbit/reinforcement_learning/blob/d61b87ad619781c9c1037b76486850a717868e8a/rlclientlib/live_model_impl.cc#L128
This allows the library user to specify what kind of logging they want (if any).

A second option is to let a driver pass a trace interface into the setup (factory) function that constructs the reduction

A third option used in some composable systems is a service_provider class that a component can query for the services it needs (trace logging in this case). The same service_provider would also support registering services with the provider. This would allow a driver (or client code) to implment and register trace providers and each reduction could ask for trace provider.

[jackgerrits]

I agree, it brings back a dependence on the global data structure but I think @rajan-chari explained it well. In order to replace the Arguments object I need to move some of the components out of it. I do think that if anything is allowed to exist in the global object then the tracer makes decent sense. Longer term the tracer should be passed separately to initialization or through one of the methods Rajan said.

[JohnLangford]

I agree with the end state but I'm uncomfortable with the intermediate state (i.e. extra dependence on the global data structure). This is not a big deal here, but in the future please shift towards (a) making trace_message an argument to setup (or one of the other approaches suggested) (b) making the overall change more of an atomic transaction.

…

On Wed, Dec 12, 2018 at 8:28 AM Jack Gerrits ***@***.***> wrote: I agree, it brings back a dependence on the global data structure but I think @rajan-chari <https://github.com/rajan-chari> explained it well. In order to replace the Arguments object I need to move some of the components out of it. I do think that if anything is allowed to exist in the global object then the tracer makes decent sense. Longer term the tracer should be passed separately to initialization or through one of the methods Rajan said. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1688 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE25mAF7NJEslfM49CHEnKcdtWsPLq7ks5u4QSTgaJpZM4ZN52P> .

[jackgerrits]

Okay that makes sense. As we work towards the larger refactoring changes that we've discussed the changes needed become prohibitively large in terms of complexity and size. As you said though we always want to stay in a state where we are functioning correctly and not left in a bad state. @rajan-chari (chime in if you disagree with me) and I will try and find a happy medium between manageable incremental changes and continuing the evolution of the codebase. I do agree with you though, we shouldn't ever be at a point which we fundamentally disagree with approach checked in.

[rajan-chari]

I was thinking about this. The intermediate states might be less than ideal. Reviewing smaller transformations is optimal from a workflow perspective. Perhaps pull requests to a refactoring branch? We can merge periodically once we are in a good intermediate state.

[JohnLangford]

Sounds good. Separate branch with incremental commits is the way I've worked informally when doing refactoring personally.

…

-John

On Wed, Dec 12, 2018 at 9:03 AM Rajan ***@***.***> wrote: I was thinking about this. The intermediate states might be less than ideal. Reviewing smaller transformations is optimal from a workflow perspective. Perhaps pull requests to a refactoring branch? We can merge periodically once we are in a good intermediate state. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1688 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE25sHqxVlAtbQryw_KNvZPwWAz-N0rks5u4QyngaJpZM4ZN52P> .