Skip to content

Expand Settings to handle different Django Architectures #62

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

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 53 additions & 46 deletions django_lightweight_queue/app_settings.py
Original file line number Diff line number Diff line change
@@ -1,52 +1,59 @@
from typing import Dict, Union, Mapping, TypeVar, Callable, Optional, Sequence
from typing import Any, Dict, List, Union, Callable, Optional, Sequence

from django.conf import settings
from django.conf import settings as django_settings

from . import constants
from .types import Logger, QueueName

T = TypeVar('T')


def setting(suffix: str, default: T) -> T:
attr_name = '{}{}'.format(constants.SETTING_NAME_PREFIX, suffix)
return getattr(settings, attr_name, default)


WORKERS = setting('WORKERS', {}) # type: Dict[QueueName, int]
BACKEND = setting(
'BACKEND',
'django_lightweight_queue.backends.synchronous.SynchronousBackend',
) # type: str

LOGGER_FACTORY = setting(
'LOGGER_FACTORY',
'logging.getLogger',
) # type: Union[str, Callable[[str], Logger]]

# Allow per-queue overrides of the backend.
BACKEND_OVERRIDES = setting('BACKEND_OVERRIDES', {}) # type: Mapping[QueueName, str]

MIDDLEWARE = setting('MIDDLEWARE', (
'django_lightweight_queue.middleware.logging.LoggingMiddleware',
'django_lightweight_queue.middleware.transaction.TransactionMiddleware',
)) # type: Sequence[str]

# Apps to ignore when looking for tasks. Apps must be specified as the dotted
# name used in `INSTALLED_APPS`. This is expected to be useful when you need to
# have a file called `tasks.py` within an app, but don't want
# django-lightweight-queue to import that file.
# Note: this _doesn't_ prevent tasks being registered from these apps.
IGNORE_APPS = setting('IGNORE_APPS', ()) # type: Sequence[str]

# Backend-specific settings
REDIS_HOST = setting('REDIS_HOST', '127.0.0.1') # type: str
REDIS_PORT = setting('REDIS_PORT', 6379) # type: int
REDIS_PASSWORD = setting('REDIS_PASSWORD', None) # type: Optional[str]
REDIS_PREFIX = setting('REDIS_PREFIX', '') # type: str

ENABLE_PROMETHEUS = setting('ENABLE_PROMETHEUS', False) # type: bool
# Workers will export metrics on this port, and ports following it
PROMETHEUS_START_PORT = setting('PROMETHEUS_START_PORT', 9300) # type: int

ATOMIC_JOBS = setting('ATOMIC_JOBS', True) # type: bool
class Settings:
_uses_short_names: bool = True # used later in checking for values


class Defaults(Settings):
WORKERS: Dict[QueueName, int] = {}
BACKEND: str = 'django_lightweight_queue.backends.synchronous.SynchronousBackend'
LOGGER_FACTORY: Union[str, Callable[[str], Logger]] = 'logging.getLogger'
# Allow per-queue overrides of the backend.
BACKEND_OVERRIDES: Dict[QueueName, str] = {}
MIDDLEWARE: Sequence[str] = ('django_lightweight_queue.middleware.logging.LoggingMiddleware',)
# Apps to ignore when looking for tasks. Apps must be specified as the dotted
# name used in `INSTALLED_APPS`. This is expected to be useful when you need to
# have a file called `tasks.py` within an app, but don't want
# django-lightweight-queue to import that file.
# Note: this _doesn't_ prevent tasks being registered from these apps.
IGNORE_APPS: Sequence[str] = ()
REDIS_HOST: str = '127.0.0.1'
REDIS_PORT: int = 6379
REDIS_PASSWORD: Optional[str] = None
REDIS_PREFIX: str = ""
ENABLE_PROMETHEUS: bool = False
# Workers will export metrics on this port, and ports following it
PROMETHEUS_START_PORT: int = 9300
ATOMIC_JOBS: bool = True
Comment on lines +14 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we include these members, without values, on the base Settings type?
Combined with ensuring that the app_settings variable is typed as Settings I think would that keep type-checking at the places in the code where the settings get used, which I think we do want.

I realise we might then need to typing.cast the assignment of AppSettings, but I think that's ok if so.
I'm guessing that making Settings a protocol didn't work for some reason? (I'd assumed that would avoid the need for a cast in the assignment to app_settings, but perhaps it doesn't?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaah, trying this a bit myself I can see that if Settings is a protocol mypy doesn't like the Overrides class not having the members. If that's the blocker I think the fix is probably to type: ignore it in that one place rather than sacrificing the settings type checking throughout.

(I thiiink there might also be a route involving a LongNameAdapter (see other comment) which solves the typing more completely, though that's possibly a lot more involved as a change so happy to leave that for now if there's a simpler path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly most of this silliness is related to mypy -- if I had a quarter for each time I've googled something working on this repo so far and found an open bug ticket on the mypy repo, I'd have a very nice dinner out for myself.

I'm fine with ignoring or omitting types wherever needed as long as the values themselves are clear, but it's not my repo so it's whatever's acceptable by you all.



class AppSettings:
def __init__(self, layers: List[Settings]) -> None:
self._layers = layers

def add_layer(self, layer: Settings) -> None:
self._layers.append(layer)

def __getattr__(self, name: str) -> Any:
# reverse so that later layers override earlier ones
for layer in reversed(self._layers):
# check to see if the layer is internal or external
use_short_names = getattr(layer, "_uses_short_names", False)
attr_name = (
name
if use_short_names
else '{}{}'.format(constants.SETTING_NAME_PREFIX, name)
)
if hasattr(layer, attr_name):
return getattr(layer, attr_name)

raise AttributeError(f"Sorry, '{name}' is not a valid setting.")


app_settings = AppSettings(layers=[Defaults(), django_settings])
2 changes: 1 addition & 1 deletion django_lightweight_queue/backends/redis.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@

import redis

from .. import app_settings
from ..job import Job
from .base import BackendWithPauseResume
from ..types import QueueName, WorkerNumber
from ..utils import block_for_time
from ..app_settings import app_settings


class RedisBackend(BackendWithPauseResume):
Expand Down
2 changes: 1 addition & 1 deletion django_lightweight_queue/backends/reliable_redis.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@

import redis

from .. import app_settings
from ..job import Job
from .base import BackendWithDeduplicate, BackendWithPauseResume
from ..types import QueueName, WorkerNumber
from ..utils import block_for_time, get_worker_numbers
from ..app_settings import app_settings
from ..progress_logger import ProgressLogger, NULL_PROGRESS_LOGGER

# Work around https://github.com/python/mypy/issues/9914. Name needs to match
Expand Down
2 changes: 1 addition & 1 deletion django_lightweight_queue/exposition.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@

from prometheus_client.exposition import MetricsHandler

from . import app_settings
from .types import QueueName, WorkerNumber
from .app_settings import app_settings


def get_config_response(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

from django.core.management.base import BaseCommand, CommandParser

from ... import app_settings
from ...utils import get_backend, get_queue_counts, load_extra_config
from ...app_settings import app_settings
from ...cron_scheduler import get_cron_config


Expand Down
2 changes: 1 addition & 1 deletion django_lightweight_queue/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
import subprocess
from typing import Dict, Tuple, Callable, Optional

from . import app_settings
from .types import Logger, QueueName, WorkerNumber
from .utils import get_backend, set_process_title
from .exposition import metrics_http_server
from .app_settings import app_settings
from .machine_types import Machine
from .cron_scheduler import (
CronScheduler,
Expand Down
2 changes: 1 addition & 1 deletion django_lightweight_queue/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
Optional,
)

from . import app_settings
from .job import Job
from .types import QueueName
from .utils import get_backend, contribute_implied_queue_name
from .app_settings import app_settings

TCallable = TypeVar('TCallable', bound=Callable[..., Any])

Expand Down
15 changes: 13 additions & 2 deletions django_lightweight_queue/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@
from django.core.exceptions import MiddlewareNotUsed
from django.utils.module_loading import module_has_submodule

from . import constants, app_settings
from . import constants
from .types import Logger, QueueName, WorkerNumber
from .app_settings import Settings, app_settings

if TYPE_CHECKING:
from .backends.base import BaseBackend
Expand All @@ -33,6 +34,11 @@


def load_extra_config(file_path: str) -> None:

class Overrides(Settings):
"""Container for holding internally overridden settings."""
pass

Comment on lines +37 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding: what does this provide over using Settings() directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two reasons: it makes mypy not throw a fit and since we search by getattr, starting with an empty class means that we can add whatever settings we want and if the getattr fails then it'll fall through to the next layer.

(There was originally a different comment here but it took me a minute to remember why I did it this way.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the _uses_short_names key from the base class so that we look up the right things, and I didn't want to duplicate it in case it ever needed to change 🤷

extra_settings = imp.load_source('extra_settings', file_path)

def get_setting_names(module: object) -> Set[str]:
Expand All @@ -53,9 +59,14 @@ def with_prefix(names: Iterable[str]) -> Set[str]:
warnings.warn("Ignoring unexpected setting(s) '{}'.".format(unexpected_str))

override_names = extra_names - unexpected_names

overrides = Overrides()

for name in override_names:
short_name = name[len(constants.SETTING_NAME_PREFIX):]
setattr(app_settings, short_name, getattr(extra_settings, name))
setattr(overrides, short_name, getattr(extra_settings, name))

app_settings.add_layer(overrides)


@lru_cache()
Expand Down
2 changes: 1 addition & 1 deletion django_lightweight_queue/worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@

from django.db import connections, transaction

from . import app_settings
from .types import QueueName, WorkerNumber
from .utils import get_logger, get_backend, set_process_title
from .app_settings import app_settings
from .backends.base import BaseBackend

if app_settings.ENABLE_PROMETHEUS:
Expand Down
2 changes: 1 addition & 1 deletion tests/test_pause_resume.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def setUp(self) -> None:
# Can't use override_settings due to the copying of the settings values into
# module values at startup.
@mock.patch(
'django_lightweight_queue.app_settings.BACKEND',
'django_lightweight_queue.app_settings.Defaults.BACKEND',
new='django_lightweight_queue.backends.redis.RedisBackend',
)
def test_pause_resume(self) -> None:
Expand Down
2 changes: 1 addition & 1 deletion tests/test_reliable_redis_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def mock_workers(self, workers: Mapping[str, int]) -> Iterator[None]:
'django_lightweight_queue.utils._accepting_implied_queues',
new=False,
), unittest.mock.patch.dict(
'django_lightweight_queue.app_settings.WORKERS',
'django_lightweight_queue.app_settings.app_settings.WORKERS',
workers,
):
yield
Expand Down
4 changes: 2 additions & 2 deletions tests/test_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def mock_workers(self, workers: Mapping[str, int]) -> Iterator[None]:
'django_lightweight_queue.utils._accepting_implied_queues',
new=False,
), unittest.mock.patch.dict(
'django_lightweight_queue.app_settings.WORKERS',
'django_lightweight_queue.app_settings.Defaults.WORKERS',
workers,
):
yield
Expand All @@ -54,7 +54,7 @@ def mocked_get_path(path: str) -> Any:
return get_path(path)

patch = mock.patch(
'django_lightweight_queue.app_settings.BACKEND',
'django_lightweight_queue.app_settings.Defaults.BACKEND',
new='test-backend',
)
patch.start()
Expand Down