hotfix/Frontend sync #6327
hotfix/Frontend sync #6327
Conversation
github-actions
bot
added
bug
This reverts commit 79f1dba.
| @@ -144,3 +149,27 @@ def get_interval(value: str) -> str: | |||
| } | |||
|
|
|||
| return f"{value[:-1]}{intervals[value[-1]]}" | |||
|
|
|||
|
|
|||
| # some fmp endpoint return date in EST without a timezone, this function will parse it | |||
There was a problem hiding this comment.
They actually return the date in the timezone of the exchange, without a timezone. For example:
https://financialmodelingprep.com/api/v3/historical-chart/5min/MNDI.L?
{
"date": "2024-04-08 08:05:00",
"open": 1400.5,
"low": 1396.5,
"high": 1400.5,
"close": 1398,
"volume": 5298
},
{
"date": "2024-04-08 08:00:00",
"open": 1395,
"low": 1395,
"high": 1407.5,
"close": 1399,
"volume": 4883
}
The London Stock Exchange is open Monday through Friday from 8:00 am to 4:30 pm British Summer Time (GMT+01:00).
You are localizing that as 8:00 AM EST, which is not a correct TZ conversion.
There was a problem hiding this comment.
smh they really are the best 💀
There was a problem hiding this comment.
This is exactly the reason why it's not localized, it's better to leave it alone than to make it incorrectly tz-aware.
There was a problem hiding this comment.
then I need to figure out the current timezone per symbol, or else all dates will be treated as UTC on the frontend. RIP
There was a problem hiding this comment.
Seems like something frontend needs to deal with independently of the fetcher. These should stay tz-unaware from the fetcher.
There was a problem hiding this comment.
@deeleeramone if we have to do this on FE can you get us a list of all exchanges and the time zone they in
Ty
There was a problem hiding this comment.
Not from FMP directly, at least AFAICT. They also don't use the ISO codes for exchanges.
| return [FMPFinancialRatiosData.model_validate(d) for d in results] | ||
| results: List[FMPFinancialRatiosData] = [] | ||
| for item in data: | ||
| new_item = {to_snake_case(k).replace("ttm", ""): v for k, v in item.items()} |
There was a problem hiding this comment.
Looks like we need to add "_" to the str.replace(), and "dividend_yiel_percentage" is a specialty for the "TTM" response. It's popped because it is redundant.
There was a problem hiding this comment.
also have no idea why this one keeps failing on the test runs
lol
There was a problem hiding this comment.
nice yup done!
"dividend_yield_percentage" can be dropped. This is redundant.
PE ratio and peg ratio, maybe some others are also being duplicated.
There was a problem hiding this comment.
also have no idea why this one keeps failing on the test runs
I think this is because of way the requests are structured - the nested async requests within the callback.
openbb_platform/providers/fmp/openbb_fmp/models/index_historical.py
Outdated
Show resolved
Hide resolved
| async def response_callback( | ||
| response: ClientResponse, session: ClientSession | ||
| ) -> List[Dict]: | ||
| results: List[dict] = await response.json() # type: ignore |
There was a problem hiding this comment.
This effectively undoes the efforts to expose the actual error message from the FMP response - #6237.
Instead of clearly communicating the issue to the user:
OpenBBError: FMP Error Message -> Invalid API KEY. Feel free to create a Free API Key or visit https://site.financialmodelingprep.com/faqs?search=why-is-my-api-key-invalid for more information.
It is now a validation error that cannot be traced back to the root cause:
OpenBBError: 2 validation errors for FMPFinancialRatiosData
periodEnding
Field required [type=missing, input_value={'error_message': 'Invali... for more information.'}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.7/v/missing
fiscalPeriod
Field required [type=missing, input_value={'error_message': 'Invali... for more information.'}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.7/v/missing
Frontend sync