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

Make telegram available without telegram.ext #2324

Merged
merged 16 commits into from Jan 30, 2021
Merged

Make telegram available without telegram.ext #2324

merged 16 commits into from Jan 30, 2021

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Jan 17, 2021

Introduces setup-raw.py and requirements-raw.txt so that one can install telegram without telegram.ext.

Please also see the comments below for more details on how things are done and why

python setup-raw.py bdist_wheel produces a wheel that I can install. However, in the sdist .tar.gz both setup.py and requirements.txt are included and I can't get setuptools to include them only for setup.py and the -raw versions only for setup-raw.py. Ofc this leads to pip install ptb-raw.tag.gz installing the usual ptb (just without the ext directory) instead of pbt-raw … But IG even if we manage to include only the -raw files, pip install ptb-raw.tar.gz will fail, b/c pip is looking for a setup.py

In any case, there some things to do

  • Document how things should be installed in the README and provide another README_RAW that will be set as long_description to be rendered at pypi
  • try to move pytz from reqs-raw to reqs. In v13 pytz was introduced in connection with the JobQueue & Defaults.tzinfo and helpers.to_float_timestamp currently depends on it. In hindsight probably not a good idea …
  • Check if we can drop more dependencies for ptb-raw. Noam said that he might have some ideas. Already dropped decorator (see below)
  • Update the "How to release" section of the wiki (should only need adaption on mentions of README and to also check pip install python-telegram-bot-raw)
  • Move stuff from tg.utils to a new tg.ext.utils. E.g. WebhookHandler uses tornado and that won't be installed in ptb-raw.
  • Reduce code duplicaton in setup*.py by moving stuff to helper functions

Copy link
Member

tsnoam commented Jan 17, 2021

@Bibo-Joshi Looks good in general. :-)

I thought that it would be better if python-telegram-bot would add python-telegram-bot-raw as a dependency (instead of vendoring the same files) but maybe your approach is simpler.

Copy link
Member

tsnoam commented Jan 17, 2021

@Bibo-Joshi Oh, and we'd need to update the wiki on how to release a new version.

Copy link
Member

tsnoam commented Jan 17, 2021

@Bibo-Joshi Following up on my first comment:
What would happen if someone would try to pip install both packages?

Copy link
Member Author

@tsnoam Seems like I didn't test thoroughly enough earlier; tg/ext is still included in ptb-raw.whl. Will have to fix that before testing what happens when installing both packages

Copy link
Member

tsnoam commented Jan 17, 2021

@Bibo-Joshi Make sure to rm -rf build/ dist/
Might be left overs

@Bibo-Joshi
Copy link
Member Author

@Bibo-Joshi Following up on my first comment:
What would happen if someone would try to pip install both packages?

Tested and it seems like the union of the files, i.e. installing first ptb-raw and then ptb gives ptb and so does first installing ptb and then ptb-raw.

Copy link
Member

tsnoam commented Jan 17, 2021

@Bibo-Joshi And what happens if you just uninstall one of them afterwards?

@Bibo-Joshi
Copy link
Member Author

@Bibo-Joshi And what happens if you just uninstall one of them afterwards?

uninstalling only ptb-raw leaves tg/ext in place and deletes everthing else. uninstalling only ptb removes everything (except the dist-info). that's not perfect but to be expected given that both modules share the same name. Would defintely put a big warning "install only one!" in the readme

@Poolitzer
Copy link
Member

Big warning should be fine in this situation imo

@harshil21
Copy link
Member

Check if we can drop more dependencies for ptb-raw.

Can we drop the decorator module? It was added since inspect.getfullargspec couldn't preserve the signatures of @log methods. However, if we switch back to functools.wraps() and use inspect.signature and inspect.Parameter which seem to get the right signatures of decorated functions, we could drop the decorator module, yes?

@Bibo-Joshi
Copy link
Member Author

Check if we can drop more dependencies for ptb-raw.

Can we drop the decorator module? It was added since inspect.getfullargspec couldn't preserve the signatures of @log methods. However, if we switch back to functools.wraps() and use inspect.signature and inspect.Parameter which seem to get the right signatures of decorated functions, we could drop the decorator module, yes?

you're absolutely right 👍 Already did that locally, just didn.t pubsh the changes yet :D

@Bibo-Joshi
Copy link
Member Author

Edit: Switching to functools.partial has the side effect that for unexpected arguments the TypeError happens inside the decorator, not inside the actual function, leading to tracbacks like

Traceback (most recent call last):
  File "foo.py", line 77, in <module>
    main()
  File "foo.py", line 66, in main
    message = updater.bot.send_dice(1145108092, foo=123)
  File "…\python-telegram-bot\telegram\bot.py", line 118, in decorator
    result = func(*args, **kwargs)
TypeError: send_dice() got an unexpected keyword argument 'foo'

However, as TypeError: send_dice() … still clearly states the method, I think that's something we can live with …

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 :)

@Bibo-Joshi
Copy link
Member Author

A first test-release can be viewed at https://test.pypi.org/project/python-telegram-bot-raw/

@Bibo-Joshi
Copy link
Member Author

Oh and I extended the kwargs for setup a bit, i.e. added some links and most importantly set python_requires='>=3.6'

@Bibo-Joshi
Copy link
Member Author

All right, the latest commits remove the ptb-raw pytz dependency and move the implementation of utils.{webhookhandler, promise} to a new ext.utils. However, I created shortcuts in utils.{webhookhandler, promise} in order to maintain backwards compatibility. Importing them in ptb-raw will ofc fail, but at least utils.Promise is something I can image users importing for checks like

out = bot.send_message(…)
if isinstance(out, Promise):
   ...
else:
  ...

in the context of MessageQueue. I added a deprecation notice and we can remove the shortcuts in one of the next major releases.

While typing I see that test fail timezone related, will have to have another look …

@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review January 21, 2021 17:10
@Bibo-Joshi Bibo-Joshi mentioned this pull request Jan 21, 2021
3 tasks
README_RAW.rst Outdated Show resolved Hide resolved
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
@Bibo-Joshi Bibo-Joshi mentioned this pull request Jan 24, 2021
6 tasks
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

all in all looks good.
some minor comments, up to you how you want to handle them.

.github/workflows/readme_notifier.yml Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
setup-raw.py Show resolved Hide resolved
telegram/ext/utils/promise.py Show resolved Hide resolved
telegram/ext/utils/promise.py Show resolved Hide resolved
telegram/utils/helpers.py Show resolved Hide resolved
telegram/utils/promise.py Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

For some reason codecov results don't appear anymore, but on the codecov page it looks fine.

# Conflicts:
#	telegram/bot.py
#	telegram/utils/promise.py
#	telegram/utils/webhookhandler.py
@Bibo-Joshi Bibo-Joshi merged commit 25506f1 into master Jan 30, 2021
@Bibo-Joshi Bibo-Joshi deleted the tg-raw branch January 30, 2021 13:15
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2021
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

4 participants