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

Feature: New RQ class #1822

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Feature: New RQ class #1822

wants to merge 4 commits into from

Conversation

ccrvlh
Copy link
Collaborator

@ccrvlh ccrvlh commented Feb 18, 2023

This introduces a new RQ class, which serves a few purposes:

  • Unified API for monitoring tools
  • Being able to send jobs to multiple queues from a single object
  • Allow for management tasks from a centralized location
  • Future framework to centralized configuration
  • A simpler way for remote operations
  • Better support for programmatic actions

No tests yet, will keep as draft for feedback loop.

@codecov
Copy link

codecov bot commented Feb 18, 2023

Codecov Report

Patch coverage has no change and project coverage change: -2.37 ⚠️

Comparison is base (e92682c) 95.06% compared to head (6b2f700) 92.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1822      +/-   ##
==========================================
- Coverage   95.06%   92.70%   -2.37%     
==========================================
  Files          51       52       +1     
  Lines        7895     8096     +201     
==========================================
  Hits         7505     7505              
- Misses        390      591     +201     
Impacted Files Coverage Δ
rq/main.py 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@selwin
Copy link
Collaborator

selwin commented Feb 20, 2023

Thanks for the PR!

I really like the concept of having a centralized entry point, but I'm wary about jamming all the args and kwargs that Worker, Queue and Job support into the RQ object. The use case of this centralized entry point for RQ object is something like this:

rq = RQ(redis_url)
queue = rq.get_queue('default', ..) # accepts all kwargs that queue accepts, minus connection related ones
worker = rq.get_worker('default, ...)  # accepts all kwargs that worker accepts, minus connection related ones
all_workers = rq.get_all_workers()

I disagree with having something like this:

rq = RQ(redis_url)
rq.get_queue_size('default')

I think the above is better expressed as:

rq = RQ(redis_url)
rq.get_queue('default').size()

# Or
rq = RQ(redis_url)
queue = rq.get_queue('default')
queue.size()

The only methods we may consider supporting in the RQ class is the enqueue_* methods because those are the most used methods in application codes (but I'm still not convinced).

@ccrvlh
Copy link
Collaborator Author

ccrvlh commented Feb 20, 2023

As much as I don't really mind having more verbose convenience methods, I agree with you your suggestion makes for a nicer (and more consistent) API, I'll remove those methods.

About the args and kwargs my thoughts are: I usually favor using them when passing directly to a third party API (eg Redis connection, DB querying etc), so that is flexible for the user, and you're not in control of what they are / types etc. Other than that, I tend to prefer exposed APIs all typed and documented and having objects to move a collection of args and kwargs around internally (eg. JobArguments or TaskParams or something).

I personally think this is a more powerful way of enqueueing jobs (i've been using it in production for a while now), mainly because it allows for a single import to enqueue to multiple queues (and also allows for some nice shortcuts for common tasks like job status, requeue, etc) - so I would favor keeping the args and kwargs in this centralized class - i just feel it provides a superior DX then the enqueueing through the Queue.

@selwin
Copy link
Collaborator

selwin commented Feb 20, 2023

I personally think this is a more powerful way of enqueueing jobs (i've been using it in production for a while now), mainly because it allows for a single import to enqueue to multiple queues

Agree, I think making enqueueing jobs easer through RQ object is something we can explore. We can provide rq.enqueue_*() in the RQ object.

But having the RQ object be a kitchen sink object that represents Worker, Queue and Job would be too confusing.

@ccrvlh
Copy link
Collaborator Author

ccrvlh commented Feb 20, 2023

But having the RQ object be a kitchen sink object that represents Worker, Queue and Job would be too confusing.

Let me make some changes and we'll get back to this.

The goal was never to represent the actual objects, but rather to unify the access (which occasionally may mean having multiple args/kwargs at times). IMHO some tasks shouldn't even be in the objects themselves (.all() is a good example), but we'll iterate over this.

@robhudson
Copy link
Contributor

One of the things I have noticed when moving a project to RQ is all the arguments needed in various parts of the system. E.g. if you want to change to use a JSON serializer for your jobs, you pass in your serializer to the Queue and also to the Worker, and if you use any CLI commands remember to also pass in the options there as well.

When I've seen discussion about simplifying this or moving to a centralized object, I envision something along the lines of an RQConfig class that holds all the "settings" -- a connection, the serializer, class overrides you've set up, TTL settings you want, etc., that is then used by default when working with the Queue, Job, and Worker classes, etc.

I'm not sure if that's where this is headed or I'm not fully understanding the options, but just thought I'd share a pain point I have had and what I imagine would help alleviate it.

For example:

# File this is some central part of your project.
# e.g. project/config.py
from rq.config import RQDefaultsConfig

class RQConfig(RQDefaultsConfig):
    connection = redis_conn
    serializer = JSONSerializer
    retry = Retry(max=3)
    result_ttl = 0

The default for your project is defined in an environment variable, e.g.:

export RQ_CONFIG=project.config.RQConfig

Since you need to have a defined Redis connection at a minimum, this would be a requirement.

Having a order of overrides, would be cool:

  • Pull from config object, as defined in the environment variable.
  • If a config kwarg is passed in, which would be any RQDefaultsConfig subclass, use that over the one defined in the environment variable. This would allow multiple configurations to be defined, with one being a default, but another used explicitly where needed.
  • If kwargs are passed in, those override the above.

Use defaults from config defined by RQ_CONFIG env var:

queue = Queue()
queue.enqueue(say_hello)

Or override with another config object.

from project.config.AltConfig

queue = Queue(config=AltConfig)
queue.enqueue(say_hello)

Or override with kwargs, which would pull from the default config class, but override the specific kwargs provided.

queue = Queue(connection=other_connection)
queue.enqueue(say_hello, retry=None)

@selwin
Copy link
Collaborator

selwin commented Jun 23, 2023

When I've seen discussion about simplifying this or moving to a centralized object, I envision something along the lines of an RQConfig class that holds all the "settings" -- a connection, the serializer, class overrides you've set up, TTL settings you want, etc., that is then used by default when working with the Queue, Job, and Worker classes, etc.

Definitely, so the rq worker CLI already accepts a --config argument. It would definitely make things much simpler if we can make everything share a single config object. If we have a centralized RQ class, we can do something like:

rq = RQ(path_to_config_file=)
# or
rq = RQ(config=config)

queue = rq.get_queue()  # We can then pass in kwargs if we want to override predefined configs
worker = rq.get_worker()

I'm very much in favor of having a centralized RQ object, but prefer a path where we can merge things into master bit by bit so that we can be more thorough and thoughtful in reviewing each PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants