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 JobQueues APS-Interface more explicit #3695

Merged
merged 11 commits into from Jun 2, 2023
Merged

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented May 7, 2023

Inspired by python-telegram-bot/ptbcontrib#72 and python-telegram-bot/ptbcontrib#73

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

Copy link
Member

@lemontree210 lemontree210 left a comment

Choose a reason for hiding this comment

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

Not much I can contribute here except probably ask for more clarity as to what benefits using this new method offers.

telegram/ext/_jobqueue.py Show resolved Hide resolved
telegram/ext/_jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/_jobqueue.py Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

Another comment on this.

This PR does not ensure that the classes Job and JobQueue itself can be easily persisted/pickled. This is because

  • The ext.Job instance has a reference to the aps.Job instance and this cycling reference can be a problem
  • The JobQueue has a reference to the APS Scheduler and a weak reference to the Application both of which are surely not easy to persist

IMO making ext.{Job, JobQueue} pickable out ouf the box is a non-goal of this PR. Serializing a APS Job will still be possible with reasonable effort with the following two considerations:

  • the first argument of the APS Job callback will be the JobQueue. By storing a reference to that in the JobStore itself, it will be straight forward to re-insert that on deserializing the APS Job
  • the second argument of the APS Job callback will be the ext.Job. Instead of serializing that, one can instead serialize the attributes (callback, chat_id, …)

telegram/ext/_jobqueue.py Show resolved Hide resolved
telegram/ext/_jobqueue.py Outdated Show resolved Hide resolved
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.

great, lgtm

telegram/ext/_jobqueue.py Show resolved Hide resolved
@Bibo-Joshi Bibo-Joshi merged commit 9c8d6ef into master Jun 2, 2023
24 checks passed
@Bibo-Joshi Bibo-Joshi deleted the job-run-refactor branch June 2, 2023 20:17
@github-actions github-actions bot locked and limited conversation to collaborators Jun 10, 2023
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