Skip to content

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Jul 13, 2022

closes #3114

TODO after merge: Update wiki and transition guide.

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 Bibo-Joshi added enhancement 🛠 refactor change type: refactor 🛠 breaking change type: breaking labels Jul 13, 2022
@Bibo-Joshi Bibo-Joshi added this to the v20.0a3 milestone Jul 13, 2022
@Poolitzer Poolitzer changed the title Explicit --local mde Explicit --local mode Jul 17, 2022
@Bibo-Joshi Bibo-Joshi modified the milestones: v20.0a3, v20.0a4 Aug 27, 2022
# Conflicts:
#	telegram/_bot.py
#	telegram/ext/_applicationbuilder.py
#	telegram/ext/_extbot.py
#	tests/test_applicationbuilder.py
#	tests/test_sticker.py
@Bibo-Joshi
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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

@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review September 3, 2022 16:33
@Bibo-Joshi
Copy link
Member Author

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

@Bibo-Joshi Bibo-Joshi requested review from harshil21 and Poolitzer and removed request for harshil21 September 3, 2022 16:35
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

LGTM

@Bibo-Joshi Bibo-Joshi mentioned this pull request Sep 17, 2022
5 tasks
# Conflicts:
#	telegram/_bot.py
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.

Yes, this looks good to me!

@Bibo-Joshi Bibo-Joshi merged commit fdfbcdf into master Sep 19, 2022
@Bibo-Joshi Bibo-Joshi deleted the explicit-local-mode branch September 19, 2022 20:31
@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2022
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛠 breaking change type: breaking 🔌 enhancement pr description: enhancement 🛠 refactor change type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Improve handling of local mode & docs regarding file handling
3 participants