-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: fix eval name from driver #1623
Conversation
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
Codecov Report
@@ Coverage Diff @@
## master #1623 +/- ##
==========================================
+ Coverage 84.14% 85.08% +0.94%
==========================================
Files 127 127
Lines 6642 6699 +57
==========================================
+ Hits 5589 5700 +111
+ Misses 1053 999 -54
Continue to review full report at Codecov.
|
evaluation.op_name = f'{self.exec.__class__.__name__}@{self.exec.eval_at}' | ||
else: | ||
evaluation.op_name = self.exec.__class__.__name__ |
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.
evaluation.op_name = f'{self.exec.__class__.__name__}@{self.exec.eval_at}' | |
else: | |
evaluation.op_name = self.exec.__class__.__name__ | |
evaluation.op_name = f'{self.exec.name}@{self.exec.eval_at}' | |
else: | |
evaluation.op_name = self.exec.name |
I think we can simply use .name
attribute of the executor - one of the metas
attribute of any Executor
. Also, since eval_at
is kind of BaseRankingEvaluator
specific, the BaseRankingEvaluator
could just have the following somewhere in __init__
of post_init
:
if self.eval_at:
self.name += f`@{self.eval_at}`
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.
We could, but now changing again in Hub is too much?
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.
Hub follows core.
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.
ah yes makes sense
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.
Why this will lead to many changes in Jina Hub?
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.
no, that is my bad.
The name is very noisy for what I have seen, though
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.
the name of the executor.
:type: str
:default: class name plus a random string
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.
I think the naming is better with the current changes
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.
LGTM👍
No description provided.