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 L2S HookTask in c-library and Python bindings #1818

Merged
merged 9 commits into from
Apr 4, 2019

Conversation

lokitoth
Copy link
Member

@lokitoth lokitoth commented Apr 2, 2019

Some time ago the "hook" L2S task was broken, and subsequently explicitly disabled. This change fixes it.

The underlying issues were around setting up the pointers to hook into correctly and update to the new multi-line example API, as the code changed due to various refactorings.

Fixes #1413, though there are still issues with LDF.
Fixes #1671

@lokitoth lokitoth force-pushed the dev/Fix-SearchHookTask branch 2 times, most recently from 9499277 to 798e626 Compare April 3, 2019 14:27
vowpalwabbit/search.cc Outdated Show resolved Hide resolved
@lokitoth lokitoth requested a review from hal3 April 3, 2019 15:51
lokitoth and others added 8 commits April 3, 2019 12:02
* Also update for API-level changes (search now takes in multi_ex as
  input rather than example*)
The change to free_ptr<search> for the local variable broke the code to
  register the search pointer, as it started putting the pointer to the
  free_ptr, rather than to the search object.
@lokitoth
Copy link
Member Author

lokitoth commented Apr 4, 2019

Went over this in person with @hal3.

@JohnLangford JohnLangford merged commit f2fd541 into VowpalWabbit:master Apr 4, 2019
@JohnLangford
Copy link
Member

Merged, thanks :-)

@lokitoth lokitoth deleted the dev/Fix-SearchHookTask branch April 16, 2019 13:50
jackgerrits pushed a commit to jackgerrits/vowpal_wabbit that referenced this pull request May 15, 2019
* Remove code to disable hook task

* Make test_search build under Windows

* Also update for API-level changes (search now takes in multi_ex as
  input rather than example*)

* Fix search::setup to put right data into all.searchstr

The change to free_ptr<search> for the local variable broke the code to
  register the search pointer, as it started putting the pointer to the
  free_ptr, rather than to the search object.

* Fix libsearch.h to use the multi_ex interface correctly

* Fix test_search.cc to do string concatenation correctly

* Enable test_search to build under Windows

* Fix Python L2S HookTask binding to use multi_ex properly

* Set the options_i pointer in HookTask::task_data properly
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.

library/test_search segfault on master Hook interface
3 participants