Skip to content

Conversation

@Bibo-Joshi
Copy link
Member

Let me try to wrap up what lead us here:

  • Noam suggested to switch from ujson to orjson as optional 3rd party json library, because the latter has serveral benefits
  • orjson turned out to not play well with httpx out of the box, because orjson.dumps returns bytes instead of a string
  • investigating the different possibilities lead us to the conclusion that it may be better (in terms of maintenance and code design) if PTB did not offer a built-in support for a 3rd party json library

Hence, this PR removes support for ujson.
We don't really know how many users are actually using the ujson feature currently and we also don't have any benchmarks on how ujson actually affects the performance. If users do request the option to use a custom json library, there are a few things that we can do and that we need to consider. I'm writing them down below in some detail for future reference.

  • Where is ujson currently used, i.e. where could users want to use a 3rd party library instead?
    1. DictPersisence: This is no problem. BasePersistence is already an ABC, so any user can go ahead and implement a custom version of DictPersistence with a 3rd party json library
    2. a small part in the passport-decryption logic. This is a problem, but IMHO a neglectible one b/c passports are basically dead anyways
    3. WebhookHandler: No problem. Users can implement a custom webhook solution that uses a 3rd party json lib
    4. TelegramObject.to_json: No problem. This method is not actually used anywhere.
    5. Networking backend. This is the actually interesting point. Let's get into that
  • Networking backend: JSON DE-coding: This is currently not possible to customize. If needed, we can easily fix that by making BaseRequest._parse_json_payload a public method. Then custom implementations can override it to use a 3rd party json lib.
  • Networking backend: JSON EN-coding: This is already mostly doable already: in do_request just do something like {name: my_json_lib.dumps(value) for name, value in request_data.parameters.items()}. However, the big performance advantage of some json libraries comes from the fact that they can readily json-dump a larger variety of objects, i.e. not only lists, dicts, strings & numbers but also e.g. datetime and dataclass objects. So a user might want to skip the TelegramObject.to_dict() step. To support this, we could consider to add a propert RequestData.raw_parameters (or similar naming) through which we expose exactly those values.

@Bibo-Joshi Bibo-Joshi added the 🛠 refactor change type: refactor label May 16, 2022
@Bibo-Joshi Bibo-Joshi added this to the v20.0a1 milestone May 16, 2022
@Bibo-Joshi Bibo-Joshi requested review from Poolitzer and harshil21 May 16, 2022 19:16
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey! Looks like you edited README.rst or README_RAW.rst. I'm just a friendly reminder to apply relevant changes to both of those files :)

Copy link
Member

tsnoam commented May 16, 2022

@Bibo-Joshi Thanks!

This sums up our conversation very well.

@Bibo-Joshi Bibo-Joshi mentioned this pull request May 16, 2022
1 task
Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

Actually found a mistake \o/

@Bibo-Joshi Bibo-Joshi merged commit ca4e4c6 into master May 19, 2022
@Bibo-Joshi Bibo-Joshi deleted the drop-ujson branch May 19, 2022 10:47
@Bibo-Joshi Bibo-Joshi mentioned this pull request May 19, 2022
@MiguelX413
Copy link
Contributor

I would definitely appreciate an easy way to customize the json backend as a fan of ujson. I would like to be able to keep using it in my Telegram bots.

@Bibo-Joshi
Copy link
Member Author

Thanks for reaching out @MiguelX413. I would like to have this change published in a release before making a decision on this topic, so that we can get a better impression of the impact of this PR on the community. I hope that you can understand my reasoning.

Would you mind opening a feature request issue so that we don't forget about this?
For the time being, v20.0a0 still supports ujson and v20.0a1 with this change in it will still allow customizing the json ENcoding behavior as detailed above.

@MiguelX413
Copy link
Contributor

Thanks for reaching out @MiguelX413. I would like to have this change published in a release before making a decision on this topic, so that we can get a better impression of the impact of this PR on the community. I hope that you can understand my reasoning.

That makes sense to me. @Bibo-Joshi

Would you mind opening a feature request issue so that we don't forget about this? For the time being, v20.0a0 still supports ujson and v20.0a1 with this change in it will still allow customizing the json ENcoding behavior as detailed above.

Sure thing.

@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🛠 refactor change type: refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants