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

IbkrWsKey should have conid for market data subscriptions #18

Open
teodorkostov opened this issue Jul 16, 2024 · 7 comments
Open

IbkrWsKey should have conid for market data subscriptions #18

teodorkostov opened this issue Jul 16, 2024 · 7 comments

Comments

@teodorkostov
Copy link

Problem description

Market data subscriptions require a complex key smd+conid+{...}. The IbkrWsKey enum does not account for the possibility of having a conid as a part of the channel.

The inconsistent usage of keys in the QueueController and SubscriptionController makes this issue a bit harder to mitigate. The QueueController is using the IbkrWsKey enum as keys. While the SubscriptionController is using the key.channel string as keys.

Trying to be smart and getting the conid from the data dictionary:

class MyIbkrSubscriptionProcessor(SubscriptionProcessor):
    def make_payload(self, channel: str, data: dict) -> str:
        conid = data['conid']
        json_data = data.copy()
        del json_data['conid']

        payload = f"{channel}+{conid}+{json.dumps(json_data)}"

        return payload

    def make_subscribe_payload(self, channel: str, data: dict = None) -> str:
        return f"s{self.make_payload(channel, data)}"

    def make_unsubscribe_payload(self, channel: str, data: dict = None) -> str:
        return f"u{self.make_payload(channel, data)}"


subscription_data = {
    'conid': gc_futures[0]['conid'],
    'fields': [
        FIELD_BID_PRICE,
        FIELD_ASK_PRICE,
        FIELD_BID_SIZE,
        FIELD_ASK_SIZE,
        FIELD_LAST_PRICE,
        FIELD_VOLUME,
    ]
}
processor = MyIbkrSubscriptionProcessor()
key = IbkrWsKey.MARKET_DATA
ws_client.subscribe(channel=key.channel, data=subscription_data, subscription_processor=processor)
accessor = ws_client.new_queue_accessor(key)

results in unexpected behavior that generates additional logging:

IbkrWsClient: Handled a channel "md+712565966" message that is missing a subscription. Message: {'server_id': 'q1', 'conidEx': '712565966', 'conid': 712565966, '_updated': 1721113980282, '6119': 'q1', '85': '5', '88': '11', '86': '2452.50', '84': '2452.20', 'topic': 'smd+712565966'}
actual value from get(...): {712565966: {'conid': 712565966, '_updated': 1721113980282, 'topic': 'smd+712565966', 'market_data_marker': 'q1', 'ask_size': '5', 'bid_size': '11', 'ask_price': '2452.50', 'bid_price': '2452.20'}}

which forces the silencing:

logger = logging.getLogger('ibind.ibkr_ws_client')
logger.level = logging.ERROR

Solution

  1. QueueController and SubscriptionController should use the same keys.
  2. IbkrWsKey should provide a way to incorporate a conid for market data subscriptions

Recommendation

Add the option to skip key remapping and just return the raw message as is. If I supply field '84' (bid price) I would like to get a dictionary with key '84' and not 'bid_price'. Having the fields as constants is more than enough to write readable code. No need for the processing overhead.

@Voyz
Copy link
Owner

Voyz commented Jul 17, 2024

Hey @teodorkostov many thanks for describing your issue in detail 👍

Market data subscriptions require a complex key smd+conid+{...}. The IbkrWsKey enum does not account for the possibility of having a conid as a part of the channel.

Indeed it does require it. The solution I've been running with is to include the conid with the channel string, such as:

ws_client.subscribe(channel='mh+265598', ...)
# or
key = IbkrWsKey.MARKET_DATA
ws_client.subscribe(channel=f'{key.channel}+265598', ...)

The reason being - from standpoint of both our client and IBKR servers (as far as I could empirically see) if a conid is present, it is married into the channel name:

  • We store the channel+conid pair internally and manage subscriptions, unsubscriptions and resubsriptions based on it
  • We unsubscribe using the same channel+conid pair
  • IBKR gives us a server (or an equivalent of) for each channel+conid pair

In essence, the channel+conid pair becomes a de-facto channel name that we subscribe to. It was a choice of simplicity to treat it as just the channel argument.

You can see it being used in this form in these two examples:

Can you see anywhere in the code that would benefit from having these two stored separately?


The inconsistent usage of keys in the QueueController and SubscriptionController makes this issue a bit harder to mitigate.

This is a fair observation, and I'm glad you've pointed it out. I've gone over the code once again to see why was this the decision. Indeed, the answer is related to channel+conid pairs.

Why SubscriptionController uses strings?

  • Subscriptions make sense if we treat them as strings, containing either channel or channel+conid. A subscription to the same channel but to a different coind should be treated as a separate entry and handled individually. The unique key to a subscription is indeed the channel and its parameters.
  • Hence, the SubscriptionController uses strings - to represent channels with its parameters (if present), such as tr or mh+265598.

Why QueueController uses enums?

  • However, when we receive the data from IBKR we don't get it nicely segmented based on conid - it is just fed to us through the WebSocket. We do a little bit of parsing to figure out which channel it came from, but other than that it is just data. If I recall correctly some channels' data doesn't even contain the conid in its messages despite being needed for subscribing.
  • Hence, the part of the interface that exposes data consumption - QueueController - uses IbkrWsKey enum with a 1-1 mapping to channel names. It doesn't support channel parameters, such as conids (or channel+conid strings), since we cannot assume that all data sent to us will have it. If it does you'll need to do the parsing in your application, which usually is quite trivial. Instead of enums we could use strings that only contain channel names, but I find enums safer and more readable.

Would you have any suggestions on how to improve this?

@Voyz
Copy link
Owner

Voyz commented Jul 17, 2024

Sorry, I only just noticed your recommendation is related to a slightly different issue:

Add the option to skip key remapping and just return the raw message as is. If I supply field '84' (bid price) I would like to get a dictionary with key '84' and not 'bid_price'. Having the fields as constants is more than enough to write readable code. No need for the processing overhead.

Yes, absolutely, we can do this. Got it implemented, I'll publish it in the next release.

@teodorkostov
Copy link
Author

I do agree that it is possible to make it work. However, think about the ibind API from the perspective of a new user. The person does not know how the internals work. To make it work some advanced examples have to be read. Some of the framework code has to be explored.

It would be better for the design to guide the user.

What if the methods accept a dataclass and not the enum/string?

@dataclass(frozen=True)
class Subscription:
    key: IbkrWsKey
    conid: str = None  # or List[str]
    data: ... = None

    def __post_init__(self):
        val(self)

subscription = Subscription(...)  # should throw an error if the optional fields are not defined for the special keys
ws_client.subscribe(subscription)
accessor = ws_client.new_queue_accessor(subscription)

Then the classes could decide what key to use internally. The user of the API does not have to know.

The sooner the user gets an error that something is wrong the better.

What do you think?

@teodorkostov
Copy link
Author

Better use a library for that. Something like Pydantic. Data validation is a common problem to solve. It is a certainty that the users of an API will mess up or even exploit it. Minimizing that possibility is good choice.

@Voyz
Copy link
Owner

Voyz commented Aug 6, 2024

Sorry, I'm currently travelling, hence sparsely I find time to look into this. Things should be easing out for me in the upcoming months.

What if the methods accept a dataclass and not the enum/string?

Yeah, that seems like a good idea worth exploring. I agree that having a unified interface would be nicer.

Would you like to give it a shot implementing the change? Otherwise, I'm happy to do it once I'm back working.

@Voyz
Copy link
Owner

Voyz commented Aug 6, 2024

And as for this:

Better use a library for that. Something like Pydantic. Data validation is a common problem to solve. It is a certainty that the users of an API will mess up or even exploit it. Minimizing that possibility is good choice.

Sorry, I'm not sure if I understand what you're referring to here. Could you elaborate? You wanna do data validation for the users' input?

@Voyz
Copy link
Owner

Voyz commented Sep 11, 2024

Hey @teodorkostov just FYI I'm working on this already, you can see the WIP branch here: https://github.com/Voyz/ibind/tree/subscription-overhaul along with v0.2.0rc1 supporting the new system. I'll fill in more details once I'm more or less happy with it. Would you have time to do a PR review on it once it's ready?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants