Explicit `--local` mode

[Bibo-Joshi]

closes #3114

Checklist for PRs

• [ ] Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
• [ ] Created new or adapted existing unit tests
• [ ] Documented code changes according to the CSI standard
• [ ] Added myself alphabetically to AUTHORS.rst (optional)
• [ ] Added new classes & modules to the docs and all suitable __all__ s

[Bibo-Joshi]

I finally got around to dig into this again and with a little distance to what I've coded so far, I think it's appropriate to point out the difficulties with this PR.

Quick recap: The main idea was that introducing an argument Bot(local_mode=True/False, …) could allow both

Bot(local_mode=True).send_document(chat_id, document=path/to/file.pdf)

and

Bot(local_mode=False).send_document(chat_id, document=path/to/file.pdf)

to work, i.e. it could lift the current restriction that paths (as string or pathlib.Path) only work if the bot runs against a local bot API (which PTB is currently completely unaware of).

---

Indeed, this works fine in the setting described above. The problem are the classes InputMedia*, which accept media to be sent without having direct access to Bot.local_mode. In the worst case, the same InputMedia* object could even be passed to two different Bot objects with different local_mode setting. To make things work in this case, we have to somehow delay the processing of the media. This comes with a few complications:

1. The first place where we have both the InputMedia* object and Bot.local_mode available is in the bot method. Parsing file input is not really the job of Bot. Any handling for InputMedia* or InputFile currently takes place in RequestParameter. The current status of the PR hence adds the local_mode logic in RequestParameter. IMO this is the best place to do it, but I still don't like it overly much, because it complicates the logic even more.
2. Temporary files are a problem. With local_mode=True it should be clear that the file has to exist at least until the API request is done, as the API server has to access the file. With local_mode=False, a valid use case could be something along the lines

   with tempfile() as f:
      f.write(data_from_external_service)
      media = InputMediaAnimation(animation, thumb=f.path())
   # f.path() does no longer exist
   bot.send_media_group(chat_id, media=[media])
   

   So we have to read the data from the file directly in InputMedia* long before we know if we actually need to do that.
3. We get a problem with InputMedia*.{media, thumb}. Those attributes are defined by the Bot API and we usually try to comply with that. The current status of this PR provides properties InputMedia*.{media, thumb} that always return the local_mode=True version of the media. In addition, it adds methods InputMedia*.parse_{media, thumb}(local_mode=False) which are called by RequestParameter to actually parse the media. This discrepancy seems hacky and unclean to me.

---

So what?

IMO we have three options:

1. Go ahead despite all difficulties and try to make the best out of it.
2. Make use of Bot.local_mode only for input that's passed directly to a bot method and not for InputMedia*. Passing a file path in InputMedia* could still work when the bot is actually in local mode, we just couldn't handle them if it's not.
3. Don't go ahead with a broad support for file paths at all. In this case, the Bot.local_mode setting would have no effect at all and hence, I'd just completely close this PR.

So far I've only looked back at this thread for ~2h so I really haven't decided which version I like best. But before I go ahead with 1. and spend time of getting everything as nice as possible, I'd very much like to hear what the rest of the dev teams thinks about this :)

[Poolitzer]

I think that this is supposed to be a small improvement, easing the developing burden for devs juggling between local and non local setup. Adding bot (and all this complicated stuff you mentioned) to the Input* class(es) just to make this work for everything is out of the scope imo.

I would thus vote to not include Input* and give them a doc warning that it doesn't support local bot api instance

But what about making a LocalBotInput* class instead? This means more work for the developers of course (import the correct one, and switch if the code should support both), but then we would have the same default behavior for all classes/method (always open path/non url strings).

[harshil21]

I would agree with poolitzer and vote for option 2. Better to have clean and proper support for something than to have a half baked approach (for InputMedia*).

But what about making a LocalBotInput* class instead

It does sound like too much work/complexity for a small benefit/consistency, so I don't like it too much

[Bibo-Joshi]

All right, option 2 it is, then :) Regarding LocalBotInput*, I'm with hashil.

[Bibo-Joshi]

Okay, I think this is ready for a first round of review, then :)