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

[Feature] Charting Modularity #6477

Merged
merged 36 commits into from
Jun 18, 2024
Merged

[Feature] Charting Modularity #6477

merged 36 commits into from
Jun 18, 2024

Conversation

hjoaquim
Copy link
Contributor

@hjoaquim hjoaquim commented Jun 4, 2024

  1. Why? (1-3 sentences or a bullet point list):

    • Atm is not possible to implement views in a given extension; this means that in order to implement those, one would need to touch the source code of the charting extension, which is not desireble and adds unnecessary friction.
  2. What? (1-3 sentences or a bullet point list):

    • In this PR it's proposed an alternative implementation that extends the charting extension through the pandas accessors mechanics (already in place for extending the OBBject class).
  3. Impact (1-2 sentences or a bullet point list):

    • TBD
  4. Testing Done:

from openbb import obb
res = obb.equity.price.historical("AAPL", chart=True)
res.show()

the same but on the CLI

  1. Reviewer Notes (optional):

    • Migrated all views to their own extension and deprecated charting_router.py

@github-actions github-actions bot added enhancement Enhancement platform OpenBB Platform v4 PRs for v4 labels Jun 4, 2024
@piiq
Copy link
Contributor

piiq commented Jun 4, 2024

Does this change entail that openbb-etf will by default rely on the charting extension?

@hjoaquim
Copy link
Contributor Author

hjoaquim commented Jun 4, 2024

Does this change entail that openbb-etf will by default rely on the charting extension?

That's an oversight on me that were enthusiastic developing this and wasn't really considering dependencies tbh.

The __init__.py files should look like this instead:

"""OpenBB ETF Extension."""

try:
    from openbb_charting import Charting  # type: ignore
    from openbb_etf import etf_views
except ImportError:
    pass

With this change anything else is needed, the <extension>_views.py will never be called so the imports pointing to the charting extension aren't a problem.

@deeleeramone
Copy link
Contributor

So I was trying to checkout this branch and I wasn't able to import because of an AttributeError raised by PackageBuilder.

(cli) Danglewoods-MacBook-Pro:openbb_platform danglewood$ python -c "import openbb;openbb.build()"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/danglewood/GitHub/OpenBBTerminal/openbb_platform/openbb/__init__.py", line 39, in <module>
    _PackageBuilder(_this_dir).auto_build()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/site-packages/openbb_core/app/static/package_builder.py", line 91, in __init__
    self.route_map = PathHandler.build_route_map()
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/site-packages/openbb_core/app/static/package_builder.py", line 1279, in build_route_map
    router = RouterLoader.from_extensions()
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/site-packages/openbb_core/app/router.py", line 647, in from_extensions
    for name, entry in ExtensionLoader().core_objects.items():  # type: ignore[attr-defined]
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/site-packages/openbb_core/app/extension_loader.py", line 110, in core_objects
    self._core_objects = self._load_entry_points(
                         ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/site-packages/openbb_core/app/extension_loader.py", line 175, in _load_entry_points
    return func[group](entry_points_)  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/site-packages/openbb_core/app/extension_loader.py", line 151, in load_core
    return {
           ^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/site-packages/openbb_core/app/extension_loader.py", line 152, in <dictcomp>
    ep.name: entry for ep in eps if isinstance((entry := ep.load()), Router)
                                                         ^^^^^^^^^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/site-packages/importlib_metadata/__init__.py", line 211, in load
    module = import_module(match.group('module'))
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/danglewood/miniconda3/miniconda3/envs/cli/lib/python3.11/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/danglewood/GitHub/OpenBBTerminal/openbb_platform/extensions/crypto/openbb_crypto/__init__.py", line 6, in <module>
    from openbb_crypto import crypto_views
  File "/Users/danglewood/GitHub/OpenBBTerminal/openbb_platform/extensions/crypto/openbb_crypto/crypto_views.py", line 16, in <module>
    @ext.charting_accessor
     ^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'Extension' object has no attribute 'charting_accessor'

@hjoaquim
Copy link
Contributor Author

hjoaquim commented Jun 6, 2024

So I was trying to checkout this branch and I wasn't able to import because of an AttributeError raised by PackageBuilder.

Maybe try in a fresh env? Works perfectly on my end.

@deeleeramone
Copy link
Contributor

Now I've got it going, I find this confusing. What is the expected use for all of these view accessors? They seem largely irrelevant to the returned OBBject and it's unclear what you are supposed to do with them.

Screenshot 2024-06-06 at 9 19 25 AM

piiq
piiq previously approved these changes Jun 7, 2024
@piiq piiq dismissed their stale review June 7, 2024 09:53

Wrong tab, sorry. Still in review

@hjoaquim
Copy link
Contributor Author

hjoaquim commented Jun 7, 2024

Now I've got it going, I find this confusing. What is the expected use for all of these view accessors? They seem largely irrelevant to the returned OBBject and it's unclear what you are supposed to do with them.

The charting functions are not to be used by themselves; as we were also not using them when it was implemented on the charting_router.py, instead, it's the charting extension job to "connect the dots". For the end user it should be exactly the same as was before, i.e., you'd just need to use chart=True when available!

This is simply a different way of organizing the code, everything should still be the same UX-wise.
We can make the charting acessors private so it's explicit the user shouldn't use them (although, ultimately, you can bc it's all static methods!)

@deeleeramone
Copy link
Contributor

We can make the charting acessors private so it's explicit the user shouldn't use them (although, ultimately, you can bc it's all static methods!)

I think this will provide more clarity to users as these appear like something intended for use.

@hjoaquim
Copy link
Contributor Author

We can make the charting acessors private so it's explicit the user shouldn't use them (although, ultimately, you can bc it's all static methods!)

I think this will provide more clarity to users as these appear like something intended for use.

I'm good with that! We can expand/improve this if/when we agree it's the way to go!

@hjoaquim hjoaquim requested a review from piiq June 12, 2024 13:25
@IgorWounds
Copy link
Contributor

@hjoaquim I like this!

@hjoaquim hjoaquim added this pull request to the merge queue Jun 18, 2024
Merged via the queue into develop with commit 5c79635 Jun 18, 2024
9 checks passed
@IgorWounds IgorWounds deleted the feature/charting-modularity branch June 19, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement platform OpenBB Platform v4 PRs for v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants