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.

Sign up for GitHub

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

Jump to bottom

Multiinstance mode for multiline examples #1934

Merged
merged 28 commits into from
Aug 22, 2019
Merged

Multiinstance mode for multiline examples #1934

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
8bd9f1f
multiinstance works (dirty)
ataymano Jun 13, 2019
aa07533
clear_seq_finish / finish_example unification
ataymano Jun 13, 2019
b50feef
more oop to learner
ataymano Jun 15, 2019
40dd242
cleanup
ataymano Jun 15, 2019
9e71f1a
last line handling fix
ataymano Jun 15, 2019
00a010b
cleanup
ataymano Jun 15, 2019
be960c4
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Jun 20, 2019
153c4db
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Jun 20, 2019
7e97ab1
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Jun 27, 2019
93c778c
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Jun 27, 2019
3d9bf19
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Jun 28, 2019
748f440
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Jul 15, 2019
0b5bbc6
finish example conflict fix
ataymano Jul 15, 2019
6c9b04c
finish example conflict fix
Jul 15, 2019
5305746
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Jul 16, 2019
10e2755
Merge branch 'master' into ataymano/multiinstance_fix1
jackgerrits Jul 25, 2019
7f3d1ad
Merge branch 'master' into ataymano/multiinstance_fix1
Aug 7, 2019
3201f71
build fix (forgotten merge change)
Aug 7, 2019
c663c1c
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Aug 7, 2019
694c080
comments
Aug 12, 2019
d26d296
Merge branch 'ataymano/multiinstance_fix1' of https://github.com/atay…
Aug 12, 2019
16f6eed
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Aug 12, 2019
ab8ec1f
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Aug 13, 2019
59f1f90
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Aug 16, 2019
ff4dee4
Merge branch 'master' into ataymano/multiinstance_fix1
ataymano Aug 20, 2019
e2bd1cc
unit tests fix
Aug 20, 2019
9ac88e9
Merge branch 'ataymano/multiinstance_fix1' of https://github.com/atay…
ataymano Aug 22, 2019
f684d44
Merge branch 'master' into ataymano/multiinstance_fix1
jackgerrits Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion vowpalwabbit/cb_adf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ void finish_multiline_example(vw& all, cb_adf& data, multi_ex& ec_seq)
output_example_seq(all, data, ec_seq);
global_print_newline(all);
}
VW::clear_seq_and_finish_examples(all, ec_seq);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we no longer want to clear the sequence?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is moved to learner.cc::multi_example_handler.
clear_seq_and_finish_example did 2 things:

  • finish examples (only if example is from certain vw instance's ring)
  • clear sequence without any condition.
    So,
  1. in general, it is potential memory leak (if ec_seq is the only reference to non-ring examples).
  2. particularly for multiinstance execution, it breaks the trick with using instance that allocated example as the last one.

Copy link
Member Author

@ataymano ataymano Jun 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Also, ec_seq is std vector, so there is no memory management reason for doing explicit clean. We are doing it just because the same ec_seq instance is used for every multi_line_example. But in this case it is 100% piece of driver's business logic.

VW::finish_example(all, ec_seq);
}

void finish(cb_adf& data)
Expand Down
2 changes: 1 addition & 1 deletion vowpalwabbit/cb_explore_adf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ void finish_multiline_example(vw& all, cb_explore_adf& data, multi_ex& ec_seq)
x->l.cb.costs.clear();
}

VW::clear_seq_and_finish_examples(all, ec_seq);
VW::finish_example(all, ec_seq);
}

template <bool is_learn>
Expand Down
2 changes: 1 addition & 1 deletion vowpalwabbit/cbify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ void finish_multiline_example(vw& all, cbify&, multi_ex& ec_seq)
output_example_seq(all, ec_seq);
// global_print_newline(all);
}
VW::clear_seq_and_finish_examples(all, ec_seq);
VW::finish_example(all, ec_seq);
}

base_learner* cbify_setup(options_i& options, vw& all)
Expand Down
2 changes: 1 addition & 1 deletion vowpalwabbit/csoaa.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@ void finish_multiline_example(vw& all, ldf& data, multi_ex& ec_seq)
output_example_seq(all, data, ec_seq);
global_print_newline(all);
}
VW::clear_seq_and_finish_examples(all, ec_seq);
VW::finish_example(all, ec_seq);
}

void finish(ldf& data)
Expand Down
3 changes: 1 addition & 2 deletions vowpalwabbit/example.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,13 +239,12 @@ void dealloc_example(void (*delete_label)(void*), example& ec, void (*delete_pre
void finish_example(vw&, example&);
void clean_example(vw&, example&, bool rewind);

void clear_seq_and_finish_examples(vw& all, multi_ex& ec_seq)
void finish_example(vw& all, multi_ex& ec_seq)
{
if (ec_seq.size() > 0)
for (example* ecc : ec_seq)
if (ecc->in_use)
VW::finish_example(all, *ecc);
ec_seq.clear();
}

void return_multiple_example(vw& all, v_array<example*>& examples)
Expand Down
2 changes: 0 additions & 2 deletions vowpalwabbit/example.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,5 @@ typedef std::vector<example*> multi_ex;

namespace VW
{
void clear_seq_and_finish_examples(vw& all, multi_ex& ec_seq);

void return_multiple_example(vw& all, v_array<example*>& examples);
} // namespace VW
2 changes: 1 addition & 1 deletion vowpalwabbit/explore_eval.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ void finish_multiline_example(vw& all, explore_eval& data, multi_ex& ec_seq)
output_example_seq(all, data, ec_seq);
CB_ADF::global_print_newline(all);
}
VW::clear_seq_and_finish_examples(all, ec_seq);
VW::finish_example(all, ec_seq);
}

template <bool is_learn>
Expand Down
Loading