Update tools to more gracefully handle errors#110
Conversation
* draft * complete TODOs * change order * test full responses * update tool tests * add object tests * clean up tests * remove aliases * remove from all tools * update change log * revert * Add new tools to ALL_TOOLS * draft * complete TODOs * change order * test full responses * update tool tests * add object tests * clean up tests * remove aliases * remove from all tools * update change log * revert * pull async changes * rebase * update changlog --------- Co-authored-by: Michelle Keoy <michelle.keoy@kensho.com> Co-authored-by: matthew-rosen-12 <matthewhrosen@gmail.com> Co-authored-by: jsanders-py <jordanmsanders17@gmail.com>
| periods: dict[str, EstimatesPeriodData] | ||
|
|
||
|
|
||
| class EstimatesResp(RespWithErrors): |
There was a problem hiding this comment.
I created a separate EstimatesResp model instead of using PostResponse[Estimates] because the estimates response should only contain a single result. I thought it was fine to get rid of the result key
| periods: dict[str, StatementPeriodData] # period -> statement and period data | ||
|
|
||
|
|
||
| class StatementsBatchResp(BaseModel): |
There was a problem hiding this comment.
I think this can be replaced by PostResponse[Statements] since errors exists as an attribute on PostResponse[Statements] until serialization
6edb9ab to
44849e2
Compare
44849e2 to
7addb63
Compare
6cb67e7 to
818f078
Compare
818f078 to
782354e
Compare
|
I did QA for the tools locally. The tools work as expected, except that for the below tools, identifiers without data get mapped to empty data in results: For example, the get_company_description_from_identifiers tool returns: whereas the get_consensus_target_price_from_identifiers tool returns: We could change the corresponding APIs to raise 404 errors if no data was found in order to change those empty data-mappings to an error; no additional changes to the client would be needed. |
| result: ConsensusTargetPrice | None = None | ||
| error: str | None = None | ||
|
|
||
| @model_serializer(mode="wrap") |
There was a problem hiding this comment.
these two functions get defined a few times.
Could we handle them with generics? Something like
class SingleResultResp(BaseModel, Generic[T]):
"""Base response model that unwraps a single result from the API's multi-result format."""
result: T | None = None
error: str | None = None
and
class EstimatesResp(SingleResultResp[Estimates]):
Or was one of the goals of the PR to reduce the usage of PostResponse?
There was a problem hiding this comment.
Yeah I will update to handle with generics. I wasn't sure if I should abstract out a class like that since the Estimates endpoints are the only endpoints (and will continue to be the only endpoints?) that return a single response in the results / errors format
…/kfinance into mhr.estimates.errors
Update estimates tools to gracefully handle errors
Use PostResponse consistently
raise responses for status if the response does not include an errors field (all GET responses except estimates)