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

StreamFetcher POC #6459

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

StreamFetcher POC #6459

wants to merge 12 commits into from

Conversation

IgorWounds
Copy link
Contributor

@IgorWounds IgorWounds commented May 23, 2024

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

    • The OpenBB Platform had a downside of not being able to support out-of-the-box WebSocket connections.
    • It was mentioned by a couple of users of the Platform and also OpenBB Terminal Pro.
  2. What? (1-3 sentences or a bullet point list):

    • Adapt the OpenBB Platform code to support WebSocket connections.
    • Introduces the atransform_data method.
    • Introduces a Binance live crypto price data example.
    • Standardizes data on the fly.
  3. Impact (1-2 sentences or a bullet point list):

    • Allows for handling WebSockets and increases the complexity of the routers.
  4. Testing Done:

To test on the API interface do:

import requests
import json
url = "http://127.0.0.1:8000/api/v1/crypto/price/live?symbol=ethbtc&lifetime=10"

def get_json_events_sync():
    with requests.get(url, stream=True) as r:
        for line in r.iter_lines():
            yield json.loads(line.decode("utf-8"))

for event in get_json_events_sync():
    print(event)

And on the Python interface do:

from openbb import obb

ws = obb.crypto.price.live(symbol="ethbtc", lifetime=15)
async for data in ws.body_iterator:
    print(data)
  1. Reviewer Notes (optional):

    • This can be further polished by introducing a custom decorator for handling routing when multiple providers share the same WebSocket. For now, we don't need this until we see there is usage and requests for such a feature.
    • Need to check how this is handled on the CLI. @hjoaquim

@IgorWounds IgorWounds added the do not merge Label to prevent pull request merge label May 23, 2024
@github-actions github-actions bot added enhancement Enhancement platform OpenBB Platform v4 PRs for v4 labels May 23, 2024
@IgorWounds IgorWounds removed the do not merge Label to prevent pull request merge label May 27, 2024
@IgorWounds IgorWounds requested a review from hjoaquim May 27, 2024 13:23
@IgorWounds IgorWounds marked this pull request as ready for review May 27, 2024 13:23
Comment on lines 3 to 5
# import asyncio

from openbb_core.app.model.command_context import CommandContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# import asyncio
from openbb_core.app.model.command_context import CommandContext
from openbb_core.app.model.command_context import CommandContext

@hjoaquim
Copy link
Contributor

Thoughts:

  • add new provider to pyproject.toml and dev install
  • use OBBject as returning type in router (maybe add a prop to OBBject to keep the streaming fetcher)
  • the new command live should've provider as argument; should also appear on the reference

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a blocker:

Screenshot 2024-05-29 at 3 31 05 PM

@router.command(methods=["GET"])
async def live(
symbol: str = "ethbtc", lifetime: int = 10
) -> BinanceCryptoHistoricalData:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we make this a Provider Interface method? There could be any number of providers that have a WS connection, like here - https://site.financialmodelingprep.com/developer/docs#crypto-websocket

)


class BinanceCryptoHistoricalFetcher(Fetcher):
Copy link
Contributor

@deeleeramone deeleeramone May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this model name and Fetcher class is deceiving. It is not at all like the other provider/standard models with this name. This is not historical data, and the pattern itself is significantly different. It needs to be differentiated.
Like how you have added an additional atransform method, there should be something like wsconnect that accepts a URL and **kwargs where the core logic can be easily reused, like a standard model.


https://binance.p.rapidapi.com/

If you're not a developer but would still like to use Biztoc outside of the main website,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is half BizToc and half maybe Binance.

@@ -0,0 +1,19 @@
[tool.poetry]
name = "openbb-binance"
version = "1.2.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version should be 1.0.0

@@ -0,0 +1 @@
"""Biztoc Provider tests."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Biztoc -> Binance


@router.stream(methods=["GET"])
async def live(symbol: str = "ethbtc", lifetime: int = 10, tld: str = "us") -> OBBject:
"""Connect to Binance WebSocket Crypto Price data feed."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint will not generate any documentation or descriptions because it is not using the ProviderInterface.

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