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

Multiinstance mode for multiline examples #1934

Merged
merged 28 commits into from Aug 22, 2019

Conversation

@ataymano
Copy link
Member

commented Jun 15, 2019

Multiinstance mode for multiline examples
#1840

@@ -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);

This comment has been minimized.

Copy link
@jackgerrits

jackgerrits Jun 19, 2019

Member

Why do we no longer want to clear the sequence?

This comment has been minimized.

Copy link
@ataymano

ataymano Jun 19, 2019

Author Member

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.

This comment has been minimized.

Copy link
@ataymano

ataymano Jun 19, 2019

Author Member

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.

@ataymano ataymano changed the title Ataymano/multiinstance fix1 Multiinstance mode for multiline examples Jun 20, 2019

ataymano added 4 commits Jun 20, 2019
}

/* example headers have the word "shared" */
bool ec_is_example_header(example& ec)

This comment has been minimized.

Copy link
@jackgerrits

jackgerrits Jun 28, 2019

Member

I don't think this is going to work once CCB is in. But I have been running and testing with CCB for some time with an equivalent code path...

This comment has been minimized.

Copy link
@ataymano

ataymano Jun 28, 2019

Author Member

not sure if I get it - this function is not changed at all and CCB PR does not have any changes in learner.cc, so it should have exactly the same version of code for it.

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

What's the status of this?

ataymano and others added 5 commits Jul 15, 2019
ataymano@microsoft.com ataymano@microsoft.com
@JohnLangford
Copy link
Member

left a comment

  1. Has valgrind blessed this change?

  2. You are making a bunch of utility classes, but it seems like you are ending up with a more complex solution. What is the logic for each of the utility classes? And should we really have all of them?

all.l->end_examples();
}

class single_instance_context {

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Jul 25, 2019

Member

What is the goal with these classes?

ataymano@microsoft.com ataymano@microsoft.com
Merge branch 'master' into ataymano/multiinstance_fix1
# Conflicts:
#	vowpalwabbit/learner.cc
@ataymano

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

  1. Has valgrind blessed this change?
  2. You are making a bunch of utility classes, but it seems like you are ending up with a more complex solution. What is the logic for each of the utility classes? And should we really have all of them?
  1. Yes
  2. I do not see easier solution here. Let's discuss details offline.
ataymano@microsoft.com and others added 5 commits Aug 7, 2019
ataymano@microsoft.com ataymano@microsoft.com
Alexey Taymanov Alexey Taymanov
ataymano added 3 commits Aug 13, 2019
@JohnLangford

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

This pull request introduces 1 alert when merging ff4dee4 into 2459ec5 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

Warning - Automated code review for VowpalWabbit/vowpal_wabbit will be disabled on October 1, 2019. You can avoid this by installing the LGTM.com GitHub App. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

ataymano@microsoft.com and others added 3 commits Aug 20, 2019
ataymano@microsoft.com ataymano@microsoft.com

@jackgerrits jackgerrits merged commit 57cd4f5 into VowpalWabbit:master Aug 22, 2019

10 of 11 checks passed

MacOS CI Build #20190822.3 failed
Details
LGTM analysis: C# No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: Java No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
Linux CI Build #20190822.3 succeeded
Details
Windows CI Build #20190822.3 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 73.035%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.