-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(evaluate): fix evaluate dict merge for pydantic #1490
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks @MSNP1381 ! left some minor comments to avoid extraneous changes togpt3.py
but the changes to evaluate
LGTM
from dsp.utils.settings import settings | ||
|
||
|
||
def is_module_installed(module_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.
just curious why this is needed. does the try/except block not take care of this check if langfuse is not supported?
dsp/modules/gpt3.py
Outdated
@@ -111,6 +122,11 @@ def __init__( | |||
def _openai_client(self): | |||
return openai | |||
|
|||
# TODO: add test suite to me. |
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.
remove extra code
i just added the code comments but the merge function is unstable be cause there is varying type of output for modules. My opinion is either we just use pydantic Object or any inherited objects or user have to follow some basic interfaces (as simple as dict interface)) |
it had error when your module's output was a pydantic object rather that a dict