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

fix: enforce force reductions to add metrics in their own namespace #4506

Closed
wants to merge 1 commit into from
Closed

fix: enforce force reductions to add metrics in their own namespace #4506

wants to merge 1 commit into from

Conversation

freyam
Copy link

@freyam freyam commented Feb 23, 2023 •
edited

Fixes #4498

This pull request modifies the set_persist_metrics() function in learner.h to enforce that force reductions can only add metrics in their own namespace. The output dictionary for metrics is now a dictionary of dictionaries, where each sub-dictionary contains the metrics for a single learner. The key for each sub-dictionary is the learner's name.

Here are the steps taken to enforce this:

  1. Create a new namespace for the learner based on its name:
std::string learner_name = this->learner_ptr->_name;
std::map<std::string, float> learner_metrics;
  1. Filter out metrics from other learners and add only the learner's metrics to the new namespace:
for (auto& metric : metrics) {
  if (metric.first.find(learner_name) != std::string::npos) {
    learner_metrics[metric.first] = metric.second;
  }
}
  1. Add the learner namespace to the output dictionary:
std::map<std::string, std::map<std::string, float>> output_metrics;
output_metrics[learner_name] = learner_metrics;
  1. Call the original function pointer with the new metrics dictionary:
fn_ptr(*data, output_metrics);

Putting it all together, here's the full implementation:

LEARNER_BUILDER_DEFINE(set_persist_metrics(void (*fn_ptr)(DataT&, metric_sink&)),
  assert(fn_ptr != nullptr);
  DataT* data = this->learner_data.get();
  this->learner_ptr->_persist_metrics_f = [fn_ptr, data](metric_sink& metrics) {
    // Create a new namespace for this learner
    std::string learner_name = this->learner_ptr->name();
    std::map<std::string, float> learner_metrics;
    
    // Only add metrics from this learner to the namespace
    for (auto& metric : metrics) {
      if (metric.first.find(learner_name) != std::string::npos) {
        learner_metrics[metric.first] = metric.second;
      }
    }
    
    // Add the learner namespace to the output dictionary
    std::map<std::string, std::map<std::string, float>> output_metrics;
    output_metrics[learner_name] = learner_metrics;
    
    // Call the original function pointer with the new metrics dictionary
    fn_ptr(*data, output_metrics);
  };
)

This currently fails as metrics is not an iterable object (which makes sense from its definition). I am currently trying to figure out a workaround.

@lalo Can you review this and suggest what I could do instead of Step 2? I would love to learn from you.

@freyam freyam changed the title Enforce force reductions to add metrics in their own namespace fix: enforce force reductions to add metrics in their own namespace Feb 23, 2023
@freyam freyam marked this pull request as draft February 23, 2023 08:47
lalo
lalo reviewed Mar 3, 2023
View reviewed changes
vowpalwabbit/core/include/vw/core/learner.h
this->learner_ptr->_persist_metrics_f = [fn_ptr, data](metric_sink& metrics) { fn_ptr(*data, metrics); };
std::string learner_name = this->name;
this->learner_ptr->_persist_metrics_f = [fn_ptr, data, learner_name](metric_sink& metrics) {
metrics[learner_name] = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

here you should be able to tell metric_sink that it should push this new context called learner_name

something like

{
 metrics.push_context(learner_name);
 fn_ptr(*data, metrics);
 metrics.pop_context();
}

back inside any fn_ptr() you should be able to only operate inside this subcontext of the metrics object - you should not be able to access other contexts that might also exist.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review. This makes sense.

Copy link

Choose a reason for hiding this comment

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

Hi @lalo, I am having a question. I did not find any references for push_context and pop_context methods in the VW::metric_sink class. Am I missing something over here? Will we have to define it?

@lalo
Copy link
Collaborator

lalo commented Mar 27, 2023

Closing, feel free to re-open if you decide on tackling this. Thanks!

@lalo lalo closed this Mar 27, 2023
@RohitRathore1
Copy link

Hey @lalo,
please view this review #4506 (comment).
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lalo lalo lalo left review comments

@RohitRathore1 RohitRathore1 RohitRathore1 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

metrics namespace per reduction
3 participants
@freyam @lalo @RohitRathore1