Skip to content

Commit

Permalink
fix: recursive search for update_stats should not call `finish_exam…
Browse files Browse the repository at this point in the history
…ple` (#69)

If the `update_stats` call was skipping over reduction it was falling
back to calling `finish_example` which was incorrect behavior.
  • Loading branch information
jackgerrits authored Mar 8, 2023
1 parent 7f8728c commit c5577f1
Showing 1 changed file with 15 additions and 3 deletions.
18 changes: 15 additions & 3 deletions src/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -495,16 +495,28 @@ void update_stats_recursive(VW::workspace& workspace, LearnerT& learner, Example
const auto has_at_least_one_new_style_func = learner.has_update_stats() || learner.has_output_example_prediction() ||
learner.has_print_update() || learner.has_cleanup_example();

// Finish example used to utilize the copy forwarding semantics.
// Traverse until first hit to mimic this but with greater type safety.
// If we hit this point, there was no update stats but other funcs were
// defined so we should not forward. We log an error since this is probably an
// issue.
if (has_at_least_one_new_style_func)
{
workspace.logger.error(
"No update_stats function was registered for a reduction but other finalization functions were. This is likely "
"an issue with the reduction: '{}'. Please report this issue to the VW team.",
learner.get_name());
return;
}

// Recurse until we find a reduction with an update_stats function.
auto* base = learner.get_base_learner();
if (base != nullptr)
{
if (learner.is_multiline() != base->is_multiline())
{
THROW("Cannot forward update_stats call across multiline/singleline boundary.");
}
base->finish_example(workspace, example);

update_stats_recursive(workspace, *base, example);
}
else { THROW("No update_stats functions were registered in the stack."); }
}
Expand Down

0 comments on commit c5577f1

Please sign in to comment.