-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make background refresh optional #1655
Conversation
@@ -26,6 +26,11 @@ namespace reinforcement_learning | |||
new live_model_impl(config, fn, err_context, trace_factory, t_factory, m_factory, sender_factory)); | |||
} | |||
|
|||
live_model::live_model(live_model&& model) { | |||
_pimpl = std::move(model._pimpl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: destructor is trivial now but that might change. would a swap be better for these variables?
@@ -208,7 +208,7 @@ namespace reinforcement_learning { | |||
/** | |||
* @brief Default move constructor for live model object. | |||
*/ | |||
live_model(live_model&&) = default; | |||
live_model(live_model&&); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that move constructor is defined, move assignment should be as well
_model_download.reset(new m::model_downloader(ptransport, &_data_cb, _trace_logger.get())); | ||
return _bg_model_proc->init(_model_download.get(), status); | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we cannot update model later. Maybe it is better to add update_model method in live_model interface?
@skofsky: Are there more updates coming for this PR, or is it ready for last round of reviews + merge? |
The reinforcement_learning directory has moved to its own repo. Can you please reopen this PR there? https://github.com/VowpalWabbit/reinforcement_learning |
Thanks, I will close this and open a new pull request. |
Adding configuration option "model.backgroundrefresh" that can be set to false to disable background model loading. When background model loading is disabled the model is loaded during the live_model::init() call and never updated again.