-
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
[BugFix] Fix ReferenceGenerator Unions and Choices #6599
Conversation
reference.json
@andrewkenreich can you take a look if this breaks anything on Terminal Pro? |
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.
Left couple of comments for clarification.
Altough the choices
are now only available on the reference since they're defined at the model level instead of the field, it looks like the CLI is still able to pick them up since its defined on reference.json
.
CLI behavior seems good and choices are not conflicting between providers:
The help dialog also seems good. Take into consideration that since argparse
doesn't allow repeated arguments, the help dialog for country will have all the choices regardless the provider - the choices should not be validated at the CLI (by argparse) but instead at the Platform (by pydantic); which seems to be happening as the image above shows.
@@ -253,14 +253,22 @@ def _create_field( | |||
annotation = field.annotation | |||
|
|||
additional_description = "" | |||
choices: Dict = {} |
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.
this variable name is misleading as it doesn't take only the choices but the extra info (being it choices and multiple items allowed info)
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.
This is a local variable for iterating over choices for each the provider. The only thing ever assigned to it are choices.
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 it? line 262 is choices[p] = {"multiple_items_allowed": True, "choices": v.get("choices")}
sry if i'm misinterpreting
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.
Yes, this is a dictionary of choices for the field, by provider, where "multiple_items_allowed" is part of "choices".
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 also find this variable name ambiguous here because choices
is also used with a different meaning as a key:value pair inside the dictionary.
I suggest json_schema_extra
, since it will be used as input for that argument later
openbb_platform/providers/alpha_vantage/openbb_alpha_vantage/models/equity_historical.py
Show resolved
Hide resolved
openbb_platform/core/openbb_core/provider/standard_models/equity_historical.py
Show resolved
Hide resolved
The behaviour in CLI that is not correct is when you enter a country for EconDB that is not in the choices for OECD. Choices for OECD are not all valid choices for EconDB, and you can also enter countries far more flexibly in the EconDB provider - i.e, "us", "uk", "USA", "united_states". |
This is actually not what happens with CLI, otherwise you would be able to enter: |
I think we are good - but we can take care of it when we merge this too - wont affect us right away |
Kind of; what happens is that the CLI merges the available choices on all the providers into a unique value list. Since Very much like #6573 (tagging to reference this also on the issue) |
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'm good with this, thanks @deeleeramone for the clarifications!
Maybe would be good if @montezdesousa run through it also, since he was the one adding the multiple_items_allowed
feature
period: str = Field( | ||
default="annual", | ||
description=QUERY_DESCRIPTIONS.get("period", ""), | ||
) |
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.
why are we removing period
from this model? And others*
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.
Because this is part of the issue, docstrings and type hints do not generate correctly. Long-standing issue being resolved.
"fmp": {"multiple_items_allowed": True, "choices": None}, | ||
"polygon": {"multiple_items_allowed": True, "choices": None}, | ||
"tiingo": {"multiple_items_allowed": True, "choices": None}, | ||
"yfinance": {"multiple_items_allowed": True, "choices": None}, | ||
}, | ||
"interval": { | ||
"fmp": { | ||
"multiple_items_allowed": False, | ||
"choices": ["1m", "5m", "15m", "30m", "1h", "4h", "1d"], | ||
}, | ||
"tiingo": { | ||
"multiple_items_allowed": False, | ||
"choices": ["1m", "5m", "15m", "30m", "1h", "4h", "1d"], | ||
}, | ||
"yfinance": { | ||
"multiple_items_allowed": False, | ||
"choices": [ | ||
"1m", | ||
"2m", | ||
"5m", | ||
"15m", | ||
"30m", | ||
"60m", | ||
"90m", | ||
"1h", | ||
"1d", | ||
"5d", | ||
"1W", | ||
"1M", | ||
"1Q", | ||
], | ||
}, | ||
}, |
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.
Does this change in the choices
affect the CLI autocomplete?
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.
iiuc, no; although it won't pick from the Field
anymore, it can still pick the choices from the reference.
In this specific instance it is not entirely a show-stopper but it certainly has the potential to be. No provider should "prevail" over another, all queries should make it to the provider for it to handle/validate specifically. |
Why?:
reference.json
is creating too many Unionsreference.json
andopenapi.json
What?:
Before:
After:
Before:
After:
Impact:
__json_schema_extra__
instead of defining underField
.reference.json
more closely resemblesopenapi.json
.openapi.json
.x.price.historical
"interval" choices are now inopenapi.json
.Testing Done:
reference.json
--help
dialogues.The schema follows: Param -> schema -> (title has names of all providers) provider (if there are defined choices) -> choices