zerodha / pykiteconnect

The official Python client library for the Kite Connect trading APIs
MIT License
978 stars 467 forks source link

feat: change data structure for subscribed_tokens #124

Closed ranjanrak closed 3 years ago

ranjanrak commented 3 years ago

Have made subscribed_tokens as a list instead of the dict, so users can un-subscribe the list of current tokens in a single go using ws.unsubscribe(ws.subscribe_tokens). Have removed redundant code around subscribed_tokens. This is in response to this raised issue.

vividvilla commented 3 years ago

This is wrong. subscribe can be called any number of times so in this case it will override the previously subscribed tokens.

ranjanrak commented 3 years ago

Subscribe can be called any number of times so in this case, it will override the previously subscribed tokens.

Yes, that's what subscribe method is used for, I guess. Override, the previous set of the subscribed tokens.
Eg: ws.subscribe([new_token_list])

vividvilla commented 3 years ago

It can be called multiple times with different subscriptions. That is why we set the token to internal map self.subscribed_tokens by iterating and setting the default mode quote.

ws.subscribe([12345])
ws.subscribe([00000])
ranjanrak commented 3 years ago

That is why we set the token to internal map self.subscribed_tokens by iterating and setting the default mode quote.

I just tried the same subscription step on the old vs the new refactor code(above PR) and saw both having the same subscription default mode and accordingly the same response structure. Tried subscription step:

ws.subscribe([58790407])
ws.subscribe([58584583])

Production ticker response:

[{'tradable': True, 'mode': 'quote', 'instrument_token': 58584583, 'last_price': 4852.0, 'last_quantity': 1,
 'average_price': 4841.41, 'volume': 325, 'buy_quantity': 269, 'sell_quantity': 134, 'ohlc': {'open': 4808.0,
 'high': 4859.0, 'low': 4808.0, 'close': 4782.0}, 'change': 1.463822668339607}, 
{'tradable': True, 'mode': 'quote', 'instrument_token': 58790407, 'last_price': 48111.0, 'last_quantity': 1, 
'average_price': 47892.88, 'volume': 8864, 'buy_quantity': 616, 'sell_quantity': 721, 
'ohlc': {'open': 47720.0, 'high': 48133.0, 'low': 47713.0, 'close': 47535.0}, 'change': 1.2117387188387503}]

Refactored(above PR) ticker response:

[{'tradable': True, 'mode': 'quote', 'instrument_token': 58790407, 'last_price': 48115.0, 'last_quantity': 1, 
'average_price': 47892.17, 'volume': 8835, 'buy_quantity': 645, 'sell_quantity': 705, 
'ohlc': {'open': 47720.0, 'high': 48133.0, 'low': 47713.0, 'close': 47535.0}, 'change': 1.2201535710529083}, 
{'tradable': True, 'mode': 'quote', 'instrument_token': 58584583, 'last_price': 4852.0, 'last_quantity': 1, 
'average_price': 4841.41, 'volume': 325, 'buy_quantity': 269, 'sell_quantity': 134, 
'ohlc': {'open': 4808.0, 'high': 4859.0, 'low': 4808.0, 'close': 4782.0}, 'change': 1.463822668339607}]

I think this defaulting of subscription mode to Quote mode is already done at the ticker server end, for no mode subscribed tokens? Can you have a check once?

vividvilla commented 3 years ago

That is why we set the token to internal map self.subscribed_tokens by iterating and setting the default mode quote.

I just tried the same subscription step on the old vs the new refactor code(above PR) and saw both having the same subscription default mode and accordingly the same response structure. Tried subscription step:

ws.subscribe([58790407])
ws.subscribe([58584583])

Production ticker response:

[{'tradable': True, 'mode': 'quote', 'instrument_token': 58584583, 'last_price': 4852.0, 'last_quantity': 1,
 'average_price': 4841.41, 'volume': 325, 'buy_quantity': 269, 'sell_quantity': 134, 'ohlc': {'open': 4808.0,
 'high': 4859.0, 'low': 4808.0, 'close': 4782.0}, 'change': 1.463822668339607}, 
{'tradable': True, 'mode': 'quote', 'instrument_token': 58790407, 'last_price': 48111.0, 'last_quantity': 1, 
'average_price': 47892.88, 'volume': 8864, 'buy_quantity': 616, 'sell_quantity': 721, 
'ohlc': {'open': 47720.0, 'high': 48133.0, 'low': 47713.0, 'close': 47535.0}, 'change': 1.2117387188387503}]

Refactored(above PR) ticker response:

[{'tradable': True, 'mode': 'quote', 'instrument_token': 58790407, 'last_price': 48115.0, 'last_quantity': 1, 
'average_price': 47892.17, 'volume': 8835, 'buy_quantity': 645, 'sell_quantity': 705, 
'ohlc': {'open': 47720.0, 'high': 48133.0, 'low': 47713.0, 'close': 47535.0}, 'change': 1.2201535710529083}, 
{'tradable': True, 'mode': 'quote', 'instrument_token': 58584583, 'last_price': 4852.0, 'last_quantity': 1, 
'average_price': 4841.41, 'volume': 325, 'buy_quantity': 269, 'sell_quantity': 134, 
'ohlc': {'open': 4808.0, 'high': 4859.0, 'low': 4808.0, 'close': 4782.0}, 'change': 1.463822668339607}]

I think this defaulting of subscription mode to Quote mode is already done at the ticker server end, for no mode subscribed tokens? Can you have a check once?

The problem is not with the default subscription but resubscribe which happens to incase of any connection failure. In case of connection failure, it reconnects and restores the previous subscription from the client side by keeping a map of the token and its mode subscribed.

ranjanrak commented 3 years ago

@vividvilla

The problem is not with the default subscription but resubscribe which happens to incase of any connection failure

Actually, i had already tried this reconnection scenario at my end(manually by disconnecting the internet) and letting retry kick in a couple of times. But, the ticker response always remains the same, in both the production as well as above websocket PR. So, this scenario got me confused, whether do we maintain this instrument token list and mode at websocket server for reconnection. And, if we already maintain this at websocket server side, we can then remove this extra maintenance of setting default mode and storing complete instrument dict at the python websocket client side. But, then discussion with @rhnvrm came to know that, this auto subscription list, after reconnection(to be specific internet disconnection) is done at the OS end, as it somehow is able to use old connection and accordingly use, previous instrument list and modes :)

Fine, if instrument token and modes are maintained only in the above internet disconnection connection case. Then we should for sure, maintain instrument token dict at the client side for other re-connection scenario.