Skip to content

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

@Bibo-Joshi Bibo-Joshi added enhancement 🛠 refactor change type: refactor labels May 7, 2023
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.

@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, …)

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

@Bibo-Joshi Bibo-Joshi merged commit 9c8d6ef into master Jun 2, 2023
@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
@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

🔌 enhancement pr description: enhancement 🛠 refactor change type: refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants