Skip to content

[FEATURE] Customization of context logic #2068

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

Closed
Bibo-Joshi opened this issue Aug 25, 2020 · 7 comments
Closed

[FEATURE] Customization of context logic #2068

Bibo-Joshi opened this issue Aug 25, 2020 · 7 comments

Comments

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Aug 25, 2020

Is your feature request related to a problem? Please describe.

  • The CallbackContext introduced in v12 is a very central thing in the library and at some point people will wanna customize it, especially when using collect_additional_context in custom handlers. Also when we get typing (Type hinting #1920, due to v13), type checkers will complain setting custom attributes.
  • The persistence setup, which is tightly integrated with context, has two shortcomings:
    • Currently, on startup all data is loaded from the persistence-backend, which might slow down the startup on huge databases and even might be unneccessary. Allowing to implement a fetch-on-demand logic would solve that
    • When sharing a database with an external service, currently there is no automated way to update the data while running the process. Again, more customization of the persistence setup could solve that

Describe the solution you'd like

  1. Allow for BasePersistence.get_data() to return custom classes, defaulting to dict(). That way users can solve both of the above mentioned problems on their own (same for get_conversations). For most classes this should still be compatible with the changes introduced in Refactor Bot persistence #1994, but advanced users can use set/replace_bot() manually anyway …
  2. Allow to pass a custom subclass of CallbackContext to Updater/Dispatcher that will be used instead of CallbackContext. We should document CC.from_*() for that.
  3. Those custom classes (3 for bot/user/chat_data, 1 for context) should be passable to Updater/Dispatcher in a clean way. I.e. probably add a single parameter to pass it as tuple/dict/custom class or maybe add it to the Defaults class, although I'm not sure that's a good idea
  4. As we're introducing type hinting in v13 (Type hinting #1920), ideally the typing is clever enough to tell the handlers that they get (update: Update, context: CustomCallbackContext) instead of (update: Update, context: CallbackContext)

Describe alternatives you've considered

Additional context

Reference for devs: @JosXa @tsnoam and I had a brief discussion about some of these ideas in dev chat on 2020-08-25.

@JosXa
Copy link
Contributor

JosXa commented Aug 25, 2020

Allow to pass a custom subclass of CallbackContext to Updater/Dispatcher that will be used instead of CallbackContext. We should document CC.from_*() for that.

This subclass should act as the initializer for every library-generated CallbackContext. Maybe edit that in :)

@jomyemmanuel
Copy link

Hi @Bibo-Joshi .
I am new here and would like to contribute. I have set up python-telegram-bot repository locally as mentioned in CONTRIBUTING.rst and have gone through telegram.ext.Handler
I would like to get more information on the CallbackContext and the customization you have mentioned. Can you give me some pointers to start with?

@Poolitzer
Copy link
Member

more information on the CallbackContext

There are the docs, and there is the code. If you check both out (they are tightly integrated anyway), you should be good to go.

We cant really help you unless you ask a specific technical question :)

@Bibo-Joshi
Copy link
Member Author

I think what @Poolitzer is trying to say is that tackling this issue should probably be done by someone who is familiar with the latest releases at least from a user point of view and it might be a bit much for a first time contribution. Ofc that depends on your motivation ;)

@Bibo-Joshi Bibo-Joshi mentioned this issue Oct 15, 2020
@and3rson
Copy link

@Bibo-Joshi as per discussion on the PTB telegram group: I mentioned the possibility of replacing

if not isinstance(self.user_data, defaultdict)`  # & friends

with

if not isinstance(self.user_data, collections.abc.Mapping)`  # & friends

Are there any specific downsides with this? You're right, it "implies that we're supporting custom stuff, but that would not yet been properly implemented and tested", however I've grepped through the sources and couldn't find any places that rely on user_data/chat_data/bot_data being a defaultdict specifically. Even more, this will be backwards-compatible since people using both built-in or custom persistence classes will still be using those same defaultdicts that are constructed in current persistence implementations, but will be able to implement a broader scope of persistences by walking away from being forced to return defaultdict.

And we can still implement & test this :)

@Bibo-Joshi
Copy link
Member Author

Hey. Maybe I didn't properly explain myself or we talked past each other.
What I meant was rather that a one-line PR would not do justice to the matter. It's correct that the checks in Dispatcher around that line are the main thing to be changed in the code to allow BP.get_* to return custom stuff, but it would also need proper documentation (at least in BasePersistence, probably also on the wiki page on persistence), type hinting and testing (of the custom return stuff in BP, of the integration with Dispatcher and CallbackContext and for conversations also with ConversationHandler). If you're up for that: you're very welcome to! My point was that this is not a project for half an hour ;)
But a PR for customizing BP.get_* can indeed be independent from a PR allowing for custom CallbackContext subclasses …

PS: Please delete one of your replies - having the same comment twice does not add any value to the discussion ;)

@Bibo-Joshi Bibo-Joshi added this to the v13.2 milestone Dec 17, 2020
@Bibo-Joshi Bibo-Joshi mentioned this issue Dec 22, 2020
3 tasks
@Bibo-Joshi Bibo-Joshi modified the milestones: v13.2, v13.3 Feb 1, 2021
@Bibo-Joshi Bibo-Joshi removed this from the v13.4 milestone Feb 28, 2021
@Bibo-Joshi
Copy link
Member Author

closed by #2262

@github-actions github-actions bot locked and limited conversation to collaborators Jun 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants