Alright, worked a bit on this. Type hinting is now on a reasonably good level. some comments:

• the persistence argument to Updater/Dispatcher is only annotated as BasePersistence, without specifying the generic types, because I didn't manage to tell mypy that persistence and callback_customizer must have the same generic types. I think that's acceptable.
• ContextCustomizer got a sh*tload of overload defs for __init__. It's as ugly as it can get, but it's the only way I could achieve that type checking add_handler works correctly. IMHO that makes it worth it, especiall as ContextCustomizer is a helper class that we hopefully don't have to change often.
• A wiki page on how to type-check code using PTB is probably good anyway, this PR gives even more reason

Tests for Dispatcher and JobQueue are there. Todo: pass context_customizer to Pickle/DictPersistence and write tests to a reasonable extend.

I skipped DictPersistence, since custom classes are not json-dumpable by default, so making custom context logic work with it will take some action by the user anyway and DictPersistence as more of a starting point in the first place. But I'm ready to be convinced otherwise :D

Probably needs some reworking when #2324 is merged, i.e. moving some types from utils.types to ext.utils.types.

Hey @Bibo-Joshi, I'm just wondering what is the decision to have the dispatcher fetch the all users and chats data upon initialization and then later only use single items (per user_id and chat_id) when constructing the context and when updating persistence?
Given this there are several issues:

• on cloud function based webhook implementations when load balanced this actually could create a data concurrency issue
• it unnecessarily uses a lot more RAM (by loading unused chat/user data)

Hey @Bibo-Joshi, I'm just wondering what is the decision to have the dispatcher fetch the all users and chats data upon initialization and then later only use single items (per user_id and chat_id) when constructing the context and when updating persistence?

Sorry, I don't understand the question. Are you asking, why the default is to load everything on init? Or where in the code you can see that decision?

Hey @Bibo-Joshi, I'm just wondering what is the decision to have the dispatcher fetch the all users and chats data upon initialization and then later only use single items (per user_id and chat_id) when constructing the context and when updating persistence?

Sorry, I don't understand the question. Are you asking, why the default is to load everything on init? Or where in the code you can see that decision?

The actual decision, I've updated my comment above for the reason why that might be an issue.

Given this there are several issues:

• on cloud function based webhook implementations when load balanced this actually could create a data concurrency issue
• it unnecessarily uses a lot more RAM (by loading unused chat/user data)

I'm not sure if I fully understand the first bullet point, but if I do, then both arguments are exactly what this PR tries to improve. See #2068 for a more detailed description.

Just clarify with the example:
The webhook is served by two function instances load balanced. Imagine the case where two users contact the bot (with private chats, for simpler case). Their requests are handled concurrently with the following order:

1. User1 request hits instance 1, the data (including user 2 is loaded by instance 1) and instance 1 proceeds to handling
2. User2 request hits instance 2, the data (including user 1 is loaded by instance 2) and instance 2 proceeds to handling
3. Instance 2 completes and writes the data to persitence, i.e. a database like Postgress (i.e. using PostgressPersistence from ptb-contrib, which overwrites all user's data)
4. Instance 1 completes and hence overwrites changes to user's 2 data.

Wouldn't it be easier to have the persistence actually provide get_user_data(user_id) and just use that when constructing the context. It should simplify the codebase and not need the Dict/Mapping piece.

Though might not be backwards compatible with previous version.

Just clarify with the example:

No, this is not, what will be happening, or is currently happening. the dispatcher calls get_user_data on init, which returns a mapping, say M (currently a defaultdict). now when an update comes in, dispatcher will access M[user_id]. It's up to M to decide whether the corresponding data for this (and only this!) user has already been loaded or not and load it if necessary. After processing the update, dispatcher will call update_user_data(data, user_id). It's up to the persistence to do the updating of the data for this (and only this!) user. Analogously for chat_data.

So if something's overriden anywhere, it's the fault of either the mapping or the persistence, but never of the dispatcher ;)

Wouldn't it be easier to have the persistence actually provide get_user_data(user_id) and just use that when constructing the context. It should simplify the codebase and not need the Dict/Mapping piece.

Though might not be backwards compatible with previous version.

This one got me thinking :D It might actually be the better approach in two senses:

1. Only need to implement BasePersistence
2. Keep any db-access logic separated from Dispatcher.

To make this work with and without persistence, we'd need to refactor how dispatcher is storing user/chat_data internally. More precisely, we would need methods Dispatcher.get_user/chat_data(user/chat_data) that

• when using no persistence, return the available data/initialize it or
• when using persistence, call persistence.refresh_user/chat_data(user/chat_id, current_user/chat_data), no matter what. I named the methods refresh instead of get on purpose, b/c they should only update the existing object and not return a new one, as the user/chat_data is shared accross callbacks and it will definetely lead to problems when you start using different objects. One can argue if for users without data, dispatcher should initialize the data and pass it to persistence for refreshing or if persistence should initialize. I tend for dispatcher, as that's closer to the current setup and it emphasizes that dp is in charge of acutally managing the data.

And I guess this can be made in a non-breaking way:

• Dispatcher.chat/user_data can stay. One might argue that they should be made private and only be accessible via DP.get_*, but that could be done with a deprecation phase
• BasePersistence.get_user/chat_data can also stay. If you want to load data only on demand, they can return an empty dict. Otherwise they can still be used to load everything at once. In fact IMO they not even need be deprecated as for small to medium sized bots, loading everything on init will still be the reasonable way to go.

My thoughts about how we should proceed:

• think about this some more
• see what @noxxious responds
• Asks for feedback from the other devs
• Try to get an opinion from Pieter, the author of persistence
• split the PRs. make this one only about CallbackContext and the type of user/bot/chat_data. make the "dynamic persistence" logic it's own thread.

Other things that came to my mind while thinking about this:

• maybe make UD, CD & BD bound by a protocal that says "initializable without args", at least when using them without persistence. Scrap that. Protcols are new only in py3.8 …

Thanks, I stand corrected on the override stuff.

And I guess the return of (user/chat)_data should be a Mapping (which is the case in this PR 💯 ) as in actually returning the protocol on how to fetch the data and not necessarily the data itself, but that should maybe be addressed in the docs.

The refresh makes sense, given not all persistence implementations commit on each update and only on flush, dp has to maintain the user data in memory, but refresh would allow the persistence to make the decision on change merges.

But why not really move the whole data storing and persistence into one component out of dp altogether (like Datastore), that could make a decision on what to do with the online data and storing it whenever it's underlying implementation needs. Dispatcher only would need to have hook calls to it.

I guess the same user/chat_data object issue across callbacks could be addressed by:

1. upon start calling store to init itself (so in could do hot/warm/cold caching if needed)
2. fetching the data from store upon receiving update and constructing the context
3. run all the callbacks that need to run for this update cycle using that context and data instance
4. notify the data store of the update so it can make an appropriate decision (default implementation is just to keep it in the in dictionary)
5. Upon exit call store.flush

So persistence would be on all the time, just the default implementation would be similar to the current DictPersistence.

Realized that having BasePersistence.refresh_*_data non-abstract but raising NotImplementedError is still breaking, b/c Dispatcher will still call them. So making them pass for now.