-
Notifications
You must be signed in to change notification settings - Fork 559
Optimize validate filter scores only #3792
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
Changes from all commits
00576b5
ab8dec3
b69df5f
cd3581e
3d97759
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -633,21 +633,23 @@ def validate_and_filter_scores(self, task: AbsTask | None = None) -> Self: | |||||||||||||||||
| task = get_task(self.task_name) | ||||||||||||||||||
|
|
||||||||||||||||||
| splits = task.eval_splits | ||||||||||||||||||
| hf_subsets = task.hf_subsets | ||||||||||||||||||
| hf_subsets = set(hf_subsets) | ||||||||||||||||||
| hf_subsets = set(task.hf_subsets) # Convert to set once | ||||||||||||||||||
|
|
||||||||||||||||||
| new_scores = {} | ||||||||||||||||||
| seen_splits = set() | ||||||||||||||||||
| for split in self.scores: | ||||||||||||||||||
| if split not in splits: | ||||||||||||||||||
| continue | ||||||||||||||||||
| new_scores[split] = [] | ||||||||||||||||||
| seen_subsets = set() | ||||||||||||||||||
| for _scores in self.scores[split]: | ||||||||||||||||||
| if _scores["hf_subset"] not in hf_subsets: | ||||||||||||||||||
| continue | ||||||||||||||||||
| new_scores[split].append(_scores) | ||||||||||||||||||
| # Use list comprehension for better performance | ||||||||||||||||||
| new_scores[split] = [ | ||||||||||||||||||
| _scores | ||||||||||||||||||
| for _scores in self.scores[split] | ||||||||||||||||||
| if _scores["hf_subset"] in hf_subsets | ||||||||||||||||||
| ] | ||||||||||||||||||
| for _scores in new_scores[split]: | ||||||||||||||||||
| seen_subsets.add(_scores["hf_subset"]) | ||||||||||||||||||
|
|
||||||||||||||||||
| if seen_subsets != hf_subsets: | ||||||||||||||||||
| missing_subsets = hf_subsets - seen_subsets | ||||||||||||||||||
| if len(missing_subsets) > 2: | ||||||||||||||||||
|
|
@@ -664,9 +666,9 @@ def validate_and_filter_scores(self, task: AbsTask | None = None) -> Self: | |||||||||||||||||
| logger.warning( | ||||||||||||||||||
| f"{task.metadata.name}: Missing splits {set(splits) - seen_splits}" | ||||||||||||||||||
| ) | ||||||||||||||||||
| new_res = {**self.to_dict(), "scores": new_scores} | ||||||||||||||||||
| new_res = TaskResult.from_validated(**new_res) | ||||||||||||||||||
| return new_res | ||||||||||||||||||
| data = self.model_dump() | ||||||||||||||||||
| data["scores"] = new_scores | ||||||||||||||||||
| return type(self).model_construct(**data) | ||||||||||||||||||
|
Comment on lines
+670
to
+671
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not at all sure that this is faster - we wrote
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From validated would just use model construct too mteb/mteb/results/task_result.py Line 590 in d3bc4cc
But we can keep
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right it is the same - it is only for ModelResult and BenchmarkResult that it is different. So we assume that self.model_dump is faster than
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
mteb/mteb/results/task_result.py Lines 280 to 286 in d3bc4cc
In this part main change dict merging
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suspect as much, which is why I am not sure if this PR actually leads to a time saving that is worth the edit (it might, but I am not at all sure and if it does I would love to learn where the time saving is) |
||||||||||||||||||
|
|
||||||||||||||||||
| def is_mergeable( | ||||||||||||||||||
| self, | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this just does the loop twice - is that really faster?