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

[BUG] JobQueue.set_application drop jobstores field #3837

Closed
rejedai opened this issue Aug 11, 2023 · 8 comments · Fixed by #3913
Closed

[BUG] JobQueue.set_application drop jobstores field #3837

rejedai opened this issue Aug 11, 2023 · 8 comments · Fixed by #3913
Labels

Comments

@rejedai
Copy link

rejedai commented Aug 11, 2023

Steps to Reproduce

  1. Configure JobQueue:
job_queue = JobQueue()
job_queue.scheduler.configure(jobstores={'default': MongoDBJobStore()})
  1. Build app:
application = ApplicationBuilder().bot(tg_bot).job_queue(job_queue).build()

Expected behaviour

jobstores should should not be dropped.

Actual behaviour

After building JobQueue, jobstores are dropped.

Operating System

linux

Version of Python, python-telegram-bot & dependencies

python-telegram-bot 20.4 (2.0.3-1-g51cab98)
Bot API 6.7
Python 3.11.3 (main, Jun  5 2023, 09:32:32) [GCC 13.1.1 20230429]

Relevant log output

No response

Additional Context

No response

@Bibo-Joshi
Copy link
Member

Hi. The issue apparently is that JobQueue.set_application calls job_queue.scheduler.configure and that this apparently completely overrides the existing configuration instead of adding to it. At least I haven't found any mention in the aps docs on how to keep the current settings untouched.

A quick workaround would be

application = ApplicationBuilder().bot(tg_bot).build()
application.job_queue.add_jobstore(MongoDBJobStore()))

I do however agree that we should look into how this can be made more convenient for the (PTB) user or at the very least document this issue.

@vikram192005

This comment was marked as duplicate.

@Bibo-Joshi
Copy link
Member

Had another look at this. BaseScheduler.configure does indeed actively delete all current settings, see e.g.:

https://github.com/agronholm/apscheduler/blob/677ce711a5381c3d44f4c6313098844d26b70666/apscheduler/schedulers/base.py#L746-L747

Unfortunately, there is also no documented way to retrieve the current values of these settings (e.g. BaseScheduler._jobstores is protected so writing a wrapper that merges existing config values with new ones is not easy.

I would hence suggest that we simply add a warning to JobStore.scheduler and advice to use BaseScheduler.add/remove_* rather than BaseScheduler.configure.

@python-telegram-bot/developers would you be okay with that?

@Poolitzer
Copy link
Member

Poolitzer commented Sep 13, 2023

Not sure a warning suffices, because we will always override configure. I'd prefere to not allow this Can we subclass and deny setting it via setter? And maybe ask via an issue to get config in the future?

@Bibo-Joshi
Copy link
Member

Sorry, I don't quite understand your proposed solution. Which setter do you mean? What would you override in a subclass?
I'm not sure if asking for a change upstream is worth it, as there already are v4 pre-releases and I would imagine that the 3.x branch only gets big fixes now ...

@Poolitzer
Copy link
Member

Poolitzer commented Sep 13, 2023

Wait nevermind, how would you implement a warning anyway at .scheduler? That is not our class, thats apscheduler

@Bibo-Joshi
Copy link
Member

I meant a simple warning in the docs of JobQueue.scheduler :D

@Bibo-Joshi
Copy link
Member

PS: One could ofc make .scheduler a property and issue the warning every time it's accessed - but that would be a bit of overkill I guess

@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants