Add support for multilabel classification #324
Conversation
alex-matton
force-pushed
the
alexandre/new_classify_support
branch
2 times, most recently
from
e1c27dd to
634c298
Compare
alex-matton
requested review from
harry-cohere,
lfayoux,
mkozakov and
raphaelcristal
cohere/responses/classify.py
Outdated
| self.input = input | ||
| self.prediction = prediction | ||
| self.confidence = confidence | ||
| self.prediction = prediction # to be deprecated |
There was a problem hiding this comment.
how do you expect to deprecate this field in the future?
There was a problem hiding this comment.
@lfayoux You're probably better positioned to answer this question than me. I'm not sure what the standard procedure is for field deprecation.
There was a problem hiding this comment.
This will be deprecated when we do an api version change. Not sure if we still want to support this field in the future
There was a problem hiding this comment.
How about the following?
@property
def prediction(self):
print("your deprecation warning here")
return self.prediction
After a couple of release you can remove it altogether
There was a problem hiding this comment.
Took that into account
There was a problem hiding this comment.
Definitely no print calls, we have logger for that.
@mkozakov could you comment on versioning and if the above is an acceptable way to have breaking api and sdk changes?
There was a problem hiding this comment.
logging warnings for now is ok. when we deprecate we will need to bump the major SDK version
sanderland
left a comment
sanderland
left a comment
There was a problem hiding this comment.
Please add a test that covers this, add to changelog, and bump the minor version
|
@sanderland I implemented all your requested changes 👍. For the tests it is not possible to use a real co.classify() call as it would require to finetune a model first (which takes ~30min). Instead, I mocked the |
cohere/responses/classify.py
Outdated
|
|
||
| if self._prediction is None or self._confidence is None: | ||
| if self._prediction is not None or self._confidence is not None: | ||
| raise ValueError("Cannot have one of prediction and confidence be None and not the other one") |
There was a problem hiding this comment.
| raise ValueError("Cannot have one of prediction and confidence be None and not the other one") | |
| raise ValueError("Cannot have one of `prediction` and `confidence` be `None` and not the other one") |
cohere/responses/classify.py
Outdated
| self.input = input | ||
| self.prediction = prediction | ||
| self.confidence = confidence | ||
| self.prediction = prediction # to be deprecated |
There was a problem hiding this comment.
logging warnings for now is ok. when we deprecate we will need to bump the major SDK version
alex-matton
force-pushed
the
alexandre/new_classify_support
branch
from
acba0ea to
da18bbc
Compare
* add support for multilabel * address comments * replace mocker with monkeypatch * print -> log * address comment * remove ambiguity in comments
predictionsandconfidences. Add support for these fields.predictionandconfidencefields. Mark these fields as optional.predictionandconfidencefields will eventually be removed. Add "to be deprecated" comments.Important note: this PR does not introduce any breaking change.