-
Notifications
You must be signed in to change notification settings - Fork 3
Add helper functions to detect bad responses #31
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
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.
just a few more small comments! (also have not tested yet)
Query = str | ||
Context = str | ||
Prompt = 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.
note to self, will want to check how/if these appear in docs
context=cast(str, context), | ||
query=cast(str, query), |
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, do we need the cast(str)
here? I would've thought type checker would be smart enough to know it's a str but maybe not
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.
Don't remember why, but it complained there.
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.
Ah it seems like because we're assigning the context is not None and query is not None
to a variable and then doing the if
check, mypy is not able to realize context and query aren't null 🥲
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.
Ahh yes, now I remember!
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 one question, but looks good otherwise!
if TYPE_CHECKING: | ||
try: | ||
from cleanlab_studio.studio.trustworthy_language_model import TLM # type: ignore | ||
except ImportError: | ||
TLM = TLMProtocol | ||
else: | ||
TLM = TLMProtocol |
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.
Is there a need to do the if TYPE_CHECKING: try import TLM?
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.
I suppose not, no. I'll remove it and stick with using the type alias:
TLM = TLMProtocol
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.
Actually, what do you say about doing, the following:
- class TLMProtocol(Protocol):
+ class TLM(Protocol):
...
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 renaming the protocol class to TLM
sounds good 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.
Side note, from the from cleanlab_studio import ... # type: ignore
, I was reminded that our client libraries don't export types; we could fix that, and then we can use the exported types here. https://github.com/cleanlab/cleanlab-studio-integration/issues/1861
Files copied from #11.
Specificall, from: 49f9a9d