Skip to content
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

Asyncio #2731

Merged
merged 171 commits into from Apr 24, 2022
Merged

Asyncio #2731

merged 171 commits into from Apr 24, 2022

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Oct 14, 2021

Supersedes #2402. Closes #2288 when ready - which is not in the forseable future :D

  • ext-tests are deleted for now so that we can first concentrate on getting telegram to work
  • any changes to v14 should be merged into this ASAP

TODO

Let's use this as place to write everything down that comes to mind while working on this - will be a lot

  • move PtbRequestBase and HttpxRequest to a tg.request module/package. Maybe rename to RequestBase and HTTPXRequest
  • Improve handling of files in request - probably move that logic into a standalone method or even into Bot?
  • Update error code handling in request, use the enum from the std-lib
  • Check if we get coroutine was never awaited somewhere in the tests and also check if we can make those warnings fail the tests
  • check back with noam on whether the timeout parameter should be used for write_timeout as well in in BaseRequest
  • Check back with Noam about httpx connection pool limits: IISC on pool_timeout we want to create a new connection instead of dropping the request?!
  • Think about how we handle non-async callbacks for handlers and jobs. We might want to make sure that they run in a background thread - which is the default behavior of APscheduler

  • Think about what to do with exceptions in the long running tasks of updater. Currently we put them in the update_queue, which isn't really nice
  • persistence
  • think about a replacement for exception_event
  • a lot of tests
    • For JobQueue test that weekdays are working as expected to avoid regression when we update APS - see here and here
  • check for added # TODO comments
  • update examples. for chatmemberbot.py we can also include the changes proposed here

Things that don't need to be done in this PR

If they are not, create issues for them!

  • set socketops
  • switch to a different library for webhooks - maybe starlette (backend of fastAPI)?
  • Add transaction support to BaspePersistence, i.e. abstract methods start/end_transaction so that we can do
  1. tell persistence that we're going to start transaction
  2. hand over data
  3. tell persistence that I'm finished

Review

Thinks to look out for in a review

  • double check if we need to update docs regarding accepted types
  • Check for unwanted changes (also in tests!) - these are likely to be there because merging v14 was a mess
    • Some test_slots_behavior tests went missing for some reason. check the diff and re-add them
  • Add .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings
  • Also manually check whether thumbs are properly handled everywhere(!) where you can pass a thumb
  • take all the examples up for a test run
  • manually test webhook setups - this PR changes if the webserver listens to https://… or http://…
  • manually test how kill pid works when using App.run_* & starting the script as a background task

???

I wrote this down while working on a signal-handler based exit. not sure, if still relevant

@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Oct 14, 2021
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

@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 there. Relax, I am just a little warning for the maintainers to release directly after merging your PR, otherwise we have broken examples and people might get confused :)

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 the (dev) requirements or the pre-commit hooks. I'm just a friendly reminder to keep the pre-commit hook versions in sync with the dev requirements and the additional dependencies for the hooks in sync with the requirements :)

@Bibo-Joshi Bibo-Joshi changed the title Not even WIP: Asyncio WIP: Asyncio Feb 8, 2022
# Conflicts:
#	telegram/_files/file.py
#	telegram/ext/_basepersistence.py
#	telegram/ext/_builders.py
#	telegram/ext/_callbackqueryhandler.py
#	telegram/ext/_chatjoinrequesthandler.py
#	telegram/ext/_chatmemberhandler.py
#	telegram/ext/_choseninlineresulthandler.py
#	telegram/ext/_commandhandler.py
#	telegram/ext/_dispatcher.py
#	telegram/ext/_handler.py
#	telegram/ext/_inlinequeryhandler.py
#	telegram/ext/_messagehandler.py
#	telegram/ext/_picklepersistence.py
#	telegram/ext/_pollanswerhandler.py
#	telegram/ext/_pollhandler.py
#	telegram/ext/_precheckoutqueryhandler.py
#	telegram/ext/_shippingqueryhandler.py
#	telegram/ext/_stringcommandhandler.py
#	telegram/ext/_stringregexhandler.py
#	telegram/ext/_typehandler.py
#	telegram/ext/_updater.py
#	telegram/request.py
Copy link
Member Author

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

I scrolled through all files and tests and made a number of samller fine tunings. in the tests, I didn't notice any obvious false deletions. I guess we're down to

  1. checking thumbs support
  2. checking webhook support
  3. checking how kill pid wirks

pool wants to do 1 & 2 tomorrow, I'll have a look at 3 in a minute.
after that we can probably continue to search for doc improvements & small tweaks indefinitely - or merge 🥳

  • test_official is failing for obvious reasons
  • codacy complains about a "hard coded token" in the payment example and I haven't figured out how to mute that - IMO we can just ignore.

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.

uploading files rework review.

Btw codecov shows that quite a few lines in Application and ConversationHandler are not covered in tests?

telegram/_files/inputfile.py Outdated Show resolved Hide resolved
telegram/_bot.py Show resolved Hide resolved
telegram/request/_requestparameter.py Outdated Show resolved Hide resolved
telegram/request/_requestparameter.py Outdated Show resolved Hide resolved
telegram/request/_requestparameter.py Show resolved Hide resolved
telegram/request/_requestparameter.py Outdated Show resolved Hide resolved
telegram/request/_requestparameter.py Outdated Show resolved Hide resolved
@Bibo-Joshi Bibo-Joshi merged commit ad5a1fb into v14 Apr 24, 2022
@Bibo-Joshi Bibo-Joshi deleted the asyncio branch April 24, 2022 11:19
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants