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

feat!: don't run Celery workers in dev mode #1041

Closed
wants to merge 1 commit 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
2 changes: 2 additions & 0 deletions changelog.d/20240416_162659_arbrandes_no_workers_in_dev.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- 💥[Feature] Use Docker compose profiles to control services. (by @arbrandes) -->
- [Fix] Don't start Celery workers in dev mode, as they're never used. (by @arbrandes) -->
Copy link
Contributor

Choose a reason for hiding this comment

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

We would need some sort of guidelines to understand how the workers can be setup in dev mode, especially now that we are using docker compose profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a simple tutor dev dc start lms-worker would start the service, for instance, but the workers wouldn't be actually usable without modifying settings.py. Is this what you mean we should document?

Of course, the other question would be: why would anybody want to do this in a dev environment?

Copy link
Contributor

@DawoudSheraz DawoudSheraz Apr 22, 2024

Choose a reason for hiding this comment

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

Yeah, the command and setting changes, yes. I get the point that most devs might not be using this, we can have the doc in a followup. It is not a blocker.

12 changes: 9 additions & 3 deletions tutor/commands/compose.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@


class ComposeTaskRunner(BaseComposeTaskRunner):
def __init__(self, root: str, config: Config):
def __init__(self, root: str, config: Config, profile: str):
super().__init__(root, config)
self.profile = profile
self.project_name = ""
self.docker_compose_files: list[str] = []
self.docker_compose_job_files: list[str] = []
Expand All @@ -37,14 +38,19 @@ def docker_compose(self, *command: str) -> int:
# Note that we don't trigger the action on "run". That's because we
# don't want to trigger the action for every initialization script.
hooks.Actions.COMPOSE_PROJECT_STARTED.do(
self.root, self.config, self.project_name
self.root, self.config, self.profile, self.project_name
)
args = []
for docker_compose_path in self.docker_compose_files:
if os.path.exists(docker_compose_path):
args += ["-f", docker_compose_path]
return utils.docker_compose(
*args, "--project-name", self.project_name, *command
*args,
"--profile",
self.profile,
"--project-name",
self.project_name,
*command,
)

def run_task(self, service: str, command: str) -> int:
Expand Down
12 changes: 7 additions & 5 deletions tutor/commands/dev.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@


class DevTaskRunner(compose.ComposeTaskRunner):
def __init__(self, root: str, config: Config):
def __init__(self, root: str, config: Config, profile: str):
"""
Load docker-compose files from dev/ and local/
"""
super().__init__(root, config)
super().__init__(root, config, profile)
self.project_name = get_typed(self.config, "DEV_PROJECT_NAME", str)
self.docker_compose_files += [
tutor_env.pathjoin(self.root, "local", "docker-compose.yml"),
Expand All @@ -35,7 +35,7 @@ class DevContext(compose.BaseComposeContext):
NAME = "dev"

def job_runner(self, config: Config) -> DevTaskRunner:
return DevTaskRunner(self.root, config)
return DevTaskRunner(self.root, config, self.NAME)


@click.group(help="Run Open edX locally with development settings")
Expand All @@ -45,12 +45,14 @@ def dev(context: click.Context) -> None:


@hooks.Actions.COMPOSE_PROJECT_STARTED.add()
def _stop_on_local_start(root: str, config: Config, project_name: str) -> None:
def _stop_on_local_start(
root: str, config: Config, profile: str, project_name: str
) -> None:
"""
Stop the dev platform as soon as a platform with a different project name is
started.
"""
runner = DevTaskRunner(root, config)
runner = DevTaskRunner(root, config, profile)
if project_name != runner.project_name:
runner.docker_compose("stop")

Expand Down
12 changes: 7 additions & 5 deletions tutor/commands/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@


class LocalTaskRunner(compose.ComposeTaskRunner):
def __init__(self, root: str, config: Config):
def __init__(self, root: str, config: Config, profile: str):
"""
Load docker-compose files from local/.
"""
super().__init__(root, config)
super().__init__(root, config, profile)
self.project_name = get_typed(self.config, "LOCAL_PROJECT_NAME", str)
self.docker_compose_files += [
tutor_env.pathjoin(self.root, "local", "docker-compose.yml"),
Expand All @@ -31,7 +31,7 @@ class LocalContext(compose.BaseComposeContext):
NAME = "local"

def job_runner(self, config: Config) -> LocalTaskRunner:
return LocalTaskRunner(self.root, config)
return LocalTaskRunner(self.root, config, self.NAME)


@click.group(help="Run Open edX locally with docker-compose")
Expand All @@ -41,12 +41,14 @@ def local(context: click.Context) -> None:


@hooks.Actions.COMPOSE_PROJECT_STARTED.add()
def _stop_on_dev_start(root: str, config: Config, project_name: str) -> None:
def _stop_on_dev_start(
root: str, config: Config, profile: str, project_name: str
) -> None:
"""
Stop the local platform as soon as a platform with a different project name is
started.
"""
runner = LocalTaskRunner(root, config)
runner = LocalTaskRunner(root, config, profile)
if project_name != runner.project_name:
runner.docker_compose("stop")

Expand Down
5 changes: 3 additions & 2 deletions tutor/hooks/catalog.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def your_action():
instance, to add a callback to the :py:data:`COMPOSE_PROJECT_STARTED` action::
@hooks.Actions.COMPOSE_PROJECT_STARTED.add():
def run_this_on_start(root, config, name):
def run_this_on_start(root, config, profile, name):
print(root, config["LMS_HOST", name])
Your callback function will then be called whenever the ``COMPOSE_PROJECT_STARTED.do`` method
Expand All @@ -54,8 +54,9 @@ def run_this_on_start(root, config, name):
#:
#: :parameter str root: project root.
#: :parameter dict config: project configuration.
#: :parameter str profile: docker-compose profile.
#: :parameter str name: docker-compose project name.
COMPOSE_PROJECT_STARTED: Action[[str, Config, str]] = Action()
COMPOSE_PROJECT_STARTED: Action[[str, Config, str, str]] = Action()

#: Triggered after all interactive questions have been asked.
#: You should use this action if you want to add new questions.
Expand Down
4 changes: 4 additions & 0 deletions tutor/templates/local/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ services:
{%- endfor %}
depends_on:
- lms
profiles:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't familiar with profiles, to be honest. I like the idea of not running workers in dev, but I'd like to avoid making a breaking change to the filter API, and also changes across many python functions.

Instead, could we simply move the lms-worker and cms-worker declarations from docker-compose.yml to docker-compose.prod.yml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's going to be that simple. There are other places in the code that assume those service definitions exist. For instance, https://github.com/overhangio/tutor/blob/master/tutor/commands/compose.py#L289. Plus, I suspect profiles might be useful down the road, woudn't you say so?

If you're dead set against it, though, there is another way: we can just set the worker services in the dev compose override file to a profile like "donotstart". We get the same thing this PR provides, but without actually supporting different profiles.

- local

cms-worker:
image: {{ DOCKER_IMAGE_OPENEDX }}
Expand All @@ -189,5 +191,7 @@ services:
{%- endfor %}
depends_on:
- cms
profiles:
- local

{{ patch("local-docker-compose-services")|indent(2) }}
Loading