Add dimension support #72
Conversation
|
|
||
|
|
||
| class BaseDenseEncoder(ABC): | ||
| def __init__(self, *, dimension: Optional[int] = None, **kwargs: Any): |
There was a problem hiding this comment.
I think that if we put dimension as optional param for the base class, we should either allow it in all the child classes or not in the base at all. Right now it seems we only going to allow it for OpenAI, so maybe better to not put it in the base class? WDYT?
There was a problem hiding this comment.
I want to have dimension accessible to all the child classes since it will be weird if only OpenAI exposes. You are right in the fact that in the base init we should not receive this parameter. I create a private property with a default now and override in OpenAI now. I only define as an initialization parameter in OpenAI one.
| ) | ||
| if self._dimension: |
There was a problem hiding this comment.
either we should have here self._dimension is not None or better just assert on input that dimension > 0, right now if someone pass 0 it will work with OpenAI default which is kind of weird
| ) | ||
| if self._dimension is not None: | ||
| assert self._dimension > 0, "dimension must be a positive integer" |
There was a problem hiding this comment.
- I think it's better to check that on init and not here
- Maybe it's better to use here
self.dimensionand not the private property
Problem
OpenAI released some models that can receive dimension as a parameter. We want to support that usecase.
Solution
Added dimension property to all the base dense encoders.
Type of Change
Test Plan
Describe specific steps for validating this change.