Skip to content
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

Merged
merged 25 commits into from
Aug 5, 2024

Conversation

deeleeramone
Copy link
Contributor

@deeleeramone deeleeramone commented Jul 22, 2024

  1. Why?:

    • reference.json is creating too many Unions
    • "choices" for a provider contaminate other providers.
    • Expose "period" and "interval" parameters to reference.json and openapi.json
  2. What?:

Before:
before1

After:

after1

Before:

before2

After:

Screenshot 2024-07-22 at 7 07 05 PM

  1. Impact:

    • Where 'multiple_items_allowed', and there are defined choices, they are moved to __json_schema_extra__ instead of defining under Field.
    • Less redundant type references.
    • "choices" for one provider do not force those "choices" on other providers.
    • reference.json more closely resembles openapi.json.
    • Financial statement "period" choices are now in openapi.json.
    • x.price.historical "interval" choices are now in openapi.json.
  2. Testing Done:

    • Ran PackageBuilder tests.
    • Manual inspection of docstrings and reference.json
    • Check CLI --help dialogues.

The schema follows: Param -> schema -> (title has names of all providers) provider (if there are defined choices) -> choices

Screenshot 2024-07-24 at 2 27 23 PM

@deeleeramone deeleeramone added bug Fix bug platform OpenBB Platform v4 PRs for v4 cli OpenBB Platform CLI labels Jul 22, 2024
@deeleeramone deeleeramone changed the title [BugFix] Fix Duplicated Unions Being Generated In reference.json [BugFix] Fix ReferenceGenerator Unions and Choices Jul 23, 2024
@IgorWounds
Copy link
Contributor

@andrewkenreich can you take a look if this breaks anything on Terminal Pro?

Copy link
Contributor

@hjoaquim hjoaquim left a 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:

image

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 = {}
Copy link
Contributor

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)

Copy link
Contributor Author

@deeleeramone deeleeramone Jul 31, 2024

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.

Copy link
Contributor

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

Copy link
Contributor Author

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".

Copy link
Contributor

@montezdesousa montezdesousa Aug 5, 2024

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

@deeleeramone
Copy link
Contributor Author

deeleeramone commented Jul 31, 2024

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 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".

@deeleeramone
Copy link
Contributor Author

Thelp 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.

This is actually not what happens with CLI, otherwise you would be able to enter: economy --country us --provider econdb.

@andrewkenreich
Copy link
Contributor

@andrewkenreich can you take a look if this breaks anything on Terminal Pro?

I think we are good - but we can take care of it when we merge this too - wont affect us right away

@hjoaquim
Copy link
Contributor

hjoaquim commented Aug 2, 2024

Thelp 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.

This is actually not what happens with CLI, otherwise you would be able to enter: economy --country us --provider econdb.

Kind of; what happens is that the CLI merges the available choices on all the providers into a unique value list. Since econdb has no choices, the oecd ones prevail.
Not a showstopper imo, but definitively buggy - thanks for clarifying.

Very much like #6573 (tagging to reference this also on the issue)

Copy link
Contributor

@hjoaquim hjoaquim left a 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

Comment on lines -19 to -22
period: str = Field(
default="annual",
description=QUERY_DESCRIPTIONS.get("period", ""),
)
Copy link
Contributor

@montezdesousa montezdesousa Aug 2, 2024

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*

Copy link
Contributor Author

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.

Comment on lines +138 to +170
"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",
],
},
},
Copy link
Contributor

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?

Copy link
Contributor

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.

@deeleeramone
Copy link
Contributor Author

deeleeramone commented Aug 3, 2024

Kind of; what happens is that the CLI merges the available choices on all the providers into a unique value list. Since econdb has no choices, the oecd ones prevail. Not a showstopper imo, but definitively buggy - thanks for clarifying.

Very much like #6573 (tagging to reference this also on the issue)

In this specific instance it is not entirely a show-stopper but it certainly has the potential to be. derivatives.futures.curve comes to mind. I understand what happens, my point is that it is not desirable.

No provider should "prevail" over another, all queries should make it to the provider for it to handle/validate specifically.

@IgorWounds IgorWounds added this pull request to the merge queue Aug 5, 2024
Merged via the queue into develop with commit dd49c5f Aug 5, 2024
10 checks passed
@IgorWounds IgorWounds deleted the bugfix/reference-generator-unions branch August 8, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change bug Fix bug cli OpenBB Platform CLI platform OpenBB Platform v4 PRs for v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants