fix: add typecheck #3550
fix: add typecheck #3550
Conversation
| return type(self).model_construct(model_results=new_model_results) | ||
|
|
||
| def join_revisions(self) -> Self: | ||
| def join_revisions(self) -> "BenchmarkResults": |
There was a problem hiding this comment.
I don't think it needs the "
There was a problem hiding this comment.
I can change to this, but would require __future__
There was a problem hiding this comment.
Would prefer __future__
|
|
||
| def __iter__(self) -> Iterator[ModelResult]: | ||
| @override | ||
| def __iter__(self) -> Iterator[ModelResult]: # type: ignore[override] |
There was a problem hiding this comment.
I am not sure I get the type error here?
There was a problem hiding this comment.
BaseModel have it's own iterator
There was a problem hiding this comment.
Ahh, but it still gives you a type error even with the decorator (seems to me like we are specifying it twice)
| """ | ||
| sentences = [text for batch in inputs for text in batch["text"]] | ||
| instruction = self.get_task_instruction(task_metadata, prompt_type) | ||
| instruction: str | None = self.get_task_instruction(task_metadata, prompt_type) |
There was a problem hiding this comment.
shouldn't this be annotated by the function?
There was a problem hiding this comment.
Get task instruction will always return string, but later we have instructions = None and this cause conflict. Probably later can be changed to empty string
There was a problem hiding this comment.
Then I would probably do like this:
| instruction: str | None = self.get_task_instruction(task_metadata, prompt_type) | |
| instruction: str | None # further up | |
| instruction = self.get_task_instruction(task_metadata, prompt_type) |
otherwise we are suggesting that the function could also return None even though it can't
| exclude_aggregate: bool = False, | ||
| exclude_private: bool = False, | ||
| ) -> list[type[AbsTask]]: ... | ||
| T = TypeVar("T", AbsTask, type[AbsTask]) |
There was a problem hiding this comment.
Does this guarantee that if I provide AbsTask, then I won't get an type[AbsTask] out?
There was a problem hiding this comment.
Also how does this influence the docs?
There was a problem hiding this comment.
Does this guarantee that if I provide AbsTask, then I won't get an type[AbsTask] out?
Yes
Also how does this influence the docs?
I'll check
| def evaluate( | ||
| model: ModelMeta | MTEBModels | SentenceTransformer | CrossEncoder, | ||
| tasks: AbsTask | Iterable[AbsTask], | ||
| tasks: AbsTask | Benchmark | Iterable[AbsTask | Benchmark], |
There was a problem hiding this comment.
Can it be an iterable of benchmarks? Benchmark should be an iterable of AbsTask
There was a problem hiding this comment.
Hm, it seems that we don't support this, because I mistakenly looked to deprecated evaluator
mteb/tests/test_deprecated/test_MTEB.py
Line 16 in fe83e27
Probably we need to add a test for new evaluator too
There was a problem hiding this comment.
We can add support for it, but I am leaning toward not doing it. Keeps it cleaner and it is not like a for loop across benchmarks is hard to do.
| ) -> tuple[MTEBModels | ModelMeta, ModelMeta, ModelName, Revision]: | ||
| from sentence_transformers import CrossEncoder, SentenceTransformer | ||
|
|
||
| wrapped: MTEBModels | ModelMeta |
There was a problem hiding this comment.
| wrapped: MTEBModels | ModelMeta | |
| wrapped_model: MTEBModels | ModelMeta |
| self.tasks = list(tasks) | ||
| if len(self.tasks) > 0 and isinstance(self.tasks[0], Benchmark): | ||
| if isinstance(tasks, list) and all( | ||
| isinstance(task, Benchmark) for task in tasks | ||
| ): | ||
| self.benchmarks = tasks | ||
| self.tasks = list(chain.from_iterable(self.tasks)) |
There was a problem hiding this comment.
unsure why this was changed?
| def generate_model_card( | ||
| model_name: str, | ||
| tasks: list[AbsTask] | None = None, | ||
| tasks: Sequence[AbsTask] | None = None, |
There was a problem hiding this comment.
iterable or sequence?
| class AbsMetrics(TypedDict): | ||
| """The abstract class for the metrics returned by the tasks""" | ||
|
|
||
| ... | ||
|
|
There was a problem hiding this comment.
Unsure why this is added?
There was a problem hiding this comment.
I tried to standartize, because dict is not compatible with mappting, but I will remove this I think and change evaluate subset to Mapping, which can handle both
Co-authored-by: Kenneth Enevoldsen <[email protected]>
|
This pull request has been automatically marked as stale due to inactivity. |
If you add a model or a dataset, please add the corresponding checklist: