-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[Feature] Add the 13f data directly from the FMP #6956
base: develop
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.
Hi, thanks for the PR. It's off to a nice start, let's bring it home!
There are some FMP-related issues that become ours' without getting out ahead of them.
- For this function, the symbol input needs to accept both a ticker symbol and a CIK number. There needs to be a translation/check for this.
- obb.equity.ownership.form_13f(symbol="0001045810", provider="fmp")
- obb.equity.ownership.form_13f(symbol="NVDA", provider="fmp")
- Getting the most recent filing shouldn't require a date, and when a date is entered, it needs to find the appropriate one - i.e, a successful request shouldn't hinge on knowing the exact right date.
- We want to be able to get more than one at a time (from the same CIK, not so much multiple CIKs) so that the changes can be easily observed. The "limit" param can be a convenience method for the user to fetch the most recent N filing.
"asset_class": "titleOfClass", | ||
"filling_date": "fillingDate", | ||
"accepted_date": "acceptedDate", | ||
"ticker_cusip": "tickercusip", |
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.
"ticker_cusip" -> "symbol"
"ticker_cusip": "tickercusip", | ||
"final_link": "finalLink", | ||
} | ||
filling_date: dateType = Field( |
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.
Where fields have default=None
, they need to be annotated as, Optional[type]
query: FMPForm13FHRQueryParams, data: List[Dict], **kwargs: Any | ||
) -> List[FMPForm13FHRData]: | ||
"""Return the transformed data.""" | ||
return [FMPForm13FHRData(**d) for d in data] |
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.
FMP is returning an additional field, "cik", which we do not want because it is the filer's CIK and having this value makes things both confusing and redundant. Please pop that value before returning the response.
query, | ||
exclude=["symbol", "limit"], | ||
) | ||
result = await amake_request(url, **kwargs) |
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.
Here, we can handle API authorization errors if we pass a specific response_callback
helper function to amake_request
from openbb_fmp.utils.helpers import response_callback
result = await amake_request(url, response_callback=response_callback, **kwargs)
Obviously, you wouldn't be likely to know this, but it will ensure that authorization error messages get properly communicated to the user.
if result: | ||
results.extend(result) | ||
|
||
await asyncio.gather(*[get_one(symbol) for symbol in symbols]) |
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.
As noted already, we are more interested in getting multiple filings from the same symbol, rather than the same date for multiple symbols. If we only allow 1 symbol, but multiple dates, we'll need to handle each date for the given symbol.
|
||
|
||
@pytest.mark.record_http | ||
def test_fmp_form_13f_fetcher(credentials=test_credentials): |
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.
In, openbb_platform/extensions/equity/integration
, there are two test files for the integration tests, API and Python. A full set of parameters (can be None if the param is optional) with provider='fmp'
needs to be added to the params of the endpoint test. Insert them below the "sec" provider params.
Thanks a lot for your correction.But I still have some troubles: |
No, you'll need to use FMP endpoints for FMP data - https://site.financialmodelingprep.com/developer/docs/cik-search-company-search |
|
I understand, personally I find their documentation and labeling to be confusing, perhaps this one is better. https://site.financialmodelingprep.com/developer/docs/institutional-holders-search-api |
Yeah, I recently tried this endpoint, but it’s quite confusing. According to their documentation, you can 'search for institutional investment managers by name, ticker symbol, or CUSIP number.' However, I couldn’t get results for some of the most common symbols like 'AAPL' or 'NVDA.' |
OK, let's see what they have to say about it, we'll circle back to handling that one thing when we get an answer. Worst case scenario will be to use a field_validator and reject input to FMP that is not numeric. |
FMP replied me . https://financialmodelingprep.com/api/v4/institutional-ownership/symbol-ownership?symbol=AAPL&includeCurrentQuarter=false |
This one is not the same thing; this aggregates institutional ownership of an entity, whereas Form 13F is an entity's ownership of another. Also, it already exists here: https://docs.openbb.co/platform/reference/equity/ownership/institutional |
I konw this endpoint aggregates other data.But that's exactly what FMP tell me. If I can't use this one to get cik ,what can I use to get it? |
Something like this should work, @joshuaBri! from openbb_core.app.model.abstract.error import OpenBBError
from openbb_fmp.models.equity_profile import FMPEquityProfileFetcher
api_key = credentials.get("fmp_api_key") if credentials else ""
if query.symbol.isnumeric() is False:
try:
profile = await FMPEquityProfileFetcher.fetch_data({"symbol": query.symbol}, {"fmp_api_key": api_key})
cik = getattr(profile[0], "cik", "")
if not cik:
raise OpenBBError(f"Invalid symbol -> {query.symbol}")
except OpenBBError as e:
raise OpenBBError(f"Invalid symbol -> {query.symbol} -> {e}")
else:
cik = query.symbol The whole block, and the other imports only used within |
Thanks,I changed my codes . Please check the latest commit.@deeleeramone |
@deeleeramone |
if not dates: | ||
raise EmptyDataError("No data returned for the given symbol.") | ||
if not dates or len(dates) == 0: | ||
warn(f"Symbol Error: No data found for symbol {cik}") |
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 don't need to warn here because we are raising in the statement above.
Co-authored-by: Danglewood <[email protected]>
Co-authored-by: Danglewood <[email protected]>
Why?:
Expand the 13f data directly from the FMP
What?:
Adds fmp as a provider to, obb.equity.ownership
Impact:
Get Data of 13FHR from fmp provider