vowpal_wabbit icon indicating copy to clipboard operation
vowpal_wabbit copied to clipboard

Proposal: Split `finish_example` into `report` and `return`

Open jackgerrits opened this issue 4 years ago • 2 comments

Currently it is not possible to perform the reporting for an example without also returning the example to the example pool. This makes it very hard to build bindings which do automatic returning of the example because you are forced to give it up in finish_example. However, you still need to support returning if an example has not been learned on. So you end up needing to know if finish_example has been called on a given pointer to know if it needs to be returned or not.

Current behavior

finish_example does both reporting and returning the example to be managed by VW. It is possible to call a variant of finish_example which does not report, but you still need to keep track if the reporting version has been called or not.

https://github.com/VowpalWabbit/vowpal_wabbit/blob/c6cd3b957ed0bfc821931b128bebcfd3683d454b/vowpalwabbit/learner.h#L534

print_example was added which is the report component of this change. report_example will replace this.

Proposed expected behavior

  • Remove finish_example entirely (after deprecation)
  • Add report_example which records statistics for the given example, replaces print_example
  • Add return_example which does any necessary memory cleanup

Timeline

This will be merged for VW 9, but with finish_example deprecated and report/return being the recommended calls to make.

jackgerrits avatar Sep 28 '21 13:09 jackgerrits

@jackgerrits I want to contribute to Vopal Wabbit , can you guide me through the process as I am beginner to this . I would be really grateful :)

Yash621 avatar Sep 30 '21 19:09 Yash621

This issue is a proposal that needs to be discussed before any work should be done on it. Taking a look at issues labeled as good first issue is a good way to get started!

jackgerrits avatar Sep 30 '21 19:09 jackgerrits

This is a duplicate of and will be resolved by #4307

jackgerrits avatar Dec 21 '22 13:12 jackgerrits