Skip to content
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

fix: Added save_load to squarcb #2988

Merged
merged 17 commits into from May 11, 2021
Merged

Conversation

bassmang
Copy link
Member

@bassmang bassmang commented May 7, 2021

No description provided.

@bassmang bassmang changed the title Added save_load to squarcb fix: Added save_load to squarcb May 7, 2021
@ataymano
Copy link
Member

ataymano commented May 7, 2021

Can you please add test here?
https://github.com/VowpalWabbit/vowpal_wabbit/blob/master/test/RunTests

@bassmang
Copy link
Member Author

bassmang commented May 7, 2021

The issue here is that a vw model is not properly saved when using --squarecb because save_load was not implemented. This is how a program reacts when using when using an old vs new model (using / not using save_load to store model), and using an old vs new vw version (using / not using save_load to load model):

old model with old vw: Program runs, however does not store state of trained model
old model with new vw: Program crashes
new model with old vw: Program crashes
new model with new vw: Program runs and maintains state of trained model

test/RunTests Outdated Show resolved Hide resolved
: _counter(0)
, _gamma_scale(gamma_scale)
, _gamma_exponent(gamma_exponent)
, _elim(elim)
, _c0(c0)
, _min_cb_cost(min_cb_cost)
, _max_cb_cost(max_cb_cost)
, _model_file_version(model_file_version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid the extra copy

Suggested change
, _model_file_version(model_file_version)
, _model_file_version(std::move(model_file_version))

@jackgerrits
Copy link
Member

The format check has a pointer of how to fix it: (https://github.com/VowpalWabbit/vowpal_wabbit/pull/2988/checks?check_run_id=2547671641)

fix:
	Ensure there is an upstream remote that points to: https://github.com/VowpalWabbit/vowpal_wabbit.git
	Run: "git fetch upstream"
	Run: "git diff upstream/master...HEAD -U0 --no-color | clang-format-diff -r '^.*\.(cc|h)$' -p1 -i"

@jackgerrits jackgerrits merged commit a7891c9 into VowpalWabbit:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants