-
Notifications
You must be signed in to change notification settings - Fork 450
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/add encoder openai #1441
Fix/add encoder openai #1441
Conversation
superduperdb/ext/openai/model.py
Outdated
@@ -151,6 +153,9 @@ class OpenAIChatCompletion(OpenAI): | |||
takes_context: bool = True | |||
prompt: str = '' | |||
|
|||
def __post_init__(self): | |||
self.encoder = dtype('str') |
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.
if self.encoder is None
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.
(users could add post-processing and a custom Encoder
)
superduperdb/ext/openai/model.py
Outdated
@@ -316,6 +324,9 @@ class OpenAIImageEdit(OpenAI): | |||
takes_context: bool = True | |||
prompt: str = '' | |||
|
|||
def __post_init__(self): | |||
self.encoder = pil_image |
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 will break since the output of those models is currently bytes
. So you would need to handle that or change the default output type. I would suggest the 2nd for simplicity.
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 need to create table for ibis in post_create
func
https://github.com/SuperDuperDB/superduperdb/pull/1436#discussion_r1412303263
@@ -151,6 +153,9 @@ class OpenAIChatCompletion(OpenAI): | |||
takes_context: bool = True | |||
prompt: str = '' | |||
|
|||
def __post_init__(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.
We just need to add a default str encoder for ibis, do not need in mongodb
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.
yeah I just realised after talking with @blythed
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 could be handled with Model.pre_create
:
def pre_create(self, db):
if isinstance(db.databackend, IbisBackend) and self.Encoder is None:
self.encoder = dtype('str')
Then we can tell if we're in the ibis
situation or not.
2feb387
to
6870bd9
Compare
6870bd9
to
e468d45
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1441 +/- ##
==========================================
+ Coverage 80.33% 80.63% +0.29%
==========================================
Files 95 104 +9
Lines 6602 7235 +633
==========================================
+ Hits 5304 5834 +530
- Misses 1298 1401 +103 ☔ View full report in Codecov by Sentry. |
@@ -215,6 +222,11 @@ class OpenAIImageCreation(OpenAI): | |||
takes_context: bool = True | |||
prompt: str = '' | |||
|
|||
def __post_init__(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.
Let's use pre_create
.
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.
Please use pre_create
and use dtype
I would suggest add the logic to create the table in |
int_type = Encoder( | ||
identifier='int', | ||
encoder=lambda x: int(x), | ||
) | ||
float_type = Encoder( | ||
identifier='float', | ||
encoder=lambda x: float(x), | ||
) | ||
str_type = Encoder( | ||
identifier='str', | ||
encoder=lambda x: str(x), | ||
) | ||
|
||
bool_type = Encoder( | ||
identifier='bool', | ||
encoder=lambda x: bool(x), | ||
) |
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.
Nice, I added a discussion about this #1446
duplicate #1444 |
Description
Related Issues
Checklist
make test
successfully?Additional Notes or Comments