From e8ca3cbb938519b2c9df547d936bdf972b4e6076 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 10 May 2023 08:14:34 -0700 Subject: [PATCH 1/5] Enable blacken-docs and pydocstyle Enable blacken-docs and pydocstyle linters, as well as the trailing whitespace linter. Fix the docstrings to match pydocstyle's expectations. --- .pre-commit-config.yaml | 20 +++++++++++++++++--- pyproject.toml | 13 +++++++++++++ src/mobu/handlers/external.py | 2 ++ src/mobu/handlers/internal.py | 4 ++-- src/mobu/services/business/base.py | 17 +++++++++++------ src/mobu/services/business/nublado.py | 6 ++---- src/monkeyflocker/cli.py | 5 +---- 7 files changed, 48 insertions(+), 19 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 18495c25..a8b040e0 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -2,6 +2,7 @@ repos: - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.4.0 hooks: + - id: trailing-whitespace - id: check-yaml - id: check-toml @@ -9,15 +10,28 @@ repos: rev: 5.12.0 hooks: - id: isort - additional_dependencies: - - toml + additional_dependencies: [toml] - repo: https://github.com/psf/black rev: 23.3.0 hooks: - id: black - - repo: https://github.com/PyCQA/flake8 + - repo: https://github.com/asottile/blacken-docs + rev: 1.13.0 + hooks: + - id: blacken-docs + additional_dependencies: [black==23.3.0] + args: [-l, '79', -t, py311] + + - repo: https://github.com/pycqa/flake8 rev: 6.0.0 hooks: - id: flake8 + + - repo: https://github.com/pycqa/pydocstyle + rev: 6.3.0 + hooks: + - id: pydocstyle + additional_dependencies: [toml] + files: ^src/ diff --git a/pyproject.toml b/pyproject.toml index 05771c64..590f828a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -81,6 +81,19 @@ line_length = 79 known_first_party = ["mobu", "tests"] skip = ["docs/conf.py"] +[tool.pydocstyle] +# Reference: https://www.pydocstyle.org/en/stable/error_codes.html +convention = "numpy" +add-ignore = [ + "D105", # Missing docstring in magic method + "D104", # Missing docstring in public package + "D102", # Missing docstring in public method (docstring inheritance) + "D100", # Missing docstring in public module + # Nested classes named Config are used by Pydantic for configuration, and + # don't have a meaningful docstring. + "D106", +] + [tool.pytest.ini_options] asyncio_mode = "strict" filterwarnings = [ diff --git a/src/mobu/handlers/external.py b/src/mobu/handlers/external.py index d9bcd5f1..a61805b7 100644 --- a/src/mobu/handlers/external.py +++ b/src/mobu/handlers/external.py @@ -19,6 +19,8 @@ external_router = APIRouter() """FastAPI router for all external handlers.""" +__all__ = ["external_router"] + class FormattedJSONResponse(JSONResponse): """The same as ``fastapi.JSONResponse`` except formatted for humans.""" diff --git a/src/mobu/handlers/internal.py b/src/mobu/handlers/internal.py index 90562c73..13caae3e 100644 --- a/src/mobu/handlers/internal.py +++ b/src/mobu/handlers/internal.py @@ -12,11 +12,11 @@ from ..config import config -__all__ = ["get_index", "internal_router"] - internal_router = APIRouter() """FastAPI router for all internal handlers.""" +__all__ = ["internal_router"] + @internal_router.get( "/", diff --git a/src/mobu/services/business/base.py b/src/mobu/services/business/base.py index 168883df..09419ef6 100644 --- a/src/mobu/services/business/base.py +++ b/src/mobu/services/business/base.py @@ -105,7 +105,7 @@ async def startup(self) -> None: @abstractmethod async def execute(self) -> None: - """The business done in each loop.""" + """Execute the core of each business loop.""" async def close(self) -> None: """Clean up any allocated resources. @@ -116,7 +116,7 @@ async def close(self) -> None: pass async def shutdown(self) -> None: - """Any cleanup to do before exiting after stopping.""" + """Perform any cleanup required after stopping.""" pass # Public Business API called by the Monkey class. These methods handle the @@ -124,12 +124,14 @@ async def shutdown(self) -> None: # be overridden. async def run(self) -> None: - """The core business logic, run in a background task. + """Execute the core business logic. Calls `startup`, and then loops calling `execute` followed by `idle`, tracking failures by watching for exceptions and updating ``success_count`` and ``failure_count``. When told to stop, calls `shutdown` followed by `close`. + + This method is normally run in a background task. """ self.logger.info("Starting up...") try: @@ -162,7 +164,7 @@ async def run(self) -> None: self.control.task_done() async def run_once(self) -> None: - """The core business logic, run only once. + """Execute the core business logic, only once. Calls `startup`, `execute`, `shutdown`, and `close`. """ @@ -176,7 +178,7 @@ async def run_once(self) -> None: await self.close() async def idle(self) -> None: - """The idle pause at the end of each loop.""" + """Pause at the end of each business loop.""" self.logger.info("Idling...") with self.timings.start("idle"): await self.pause(self.options.idle_time) @@ -302,7 +304,10 @@ def dump(self) -> BusinessData: ) async def _pause_no_return(self, seconds: float) -> None: - """Same as `pause` but returns `None`. + """Pause for up to the number of seconds, handling commands. + + The same as `pause`, but returns `None`, needed for proper typing of + `iter_with_timeout`. Parameters ---------- diff --git a/src/mobu/services/business/nublado.py b/src/mobu/services/business/nublado.py index 26718bd4..fc574372 100644 --- a/src/mobu/services/business/nublado.py +++ b/src/mobu/services/business/nublado.py @@ -113,7 +113,7 @@ def __init__( @abstractmethod async def execute_code(self, session: JupyterLabSession) -> None: - """The core of the execution loop. + """Execute some code inside the Jupyter lab. Must be overridden by subclasses to use the provided lab session to perform whatever operations are desired inside the lab. If multiple @@ -158,7 +158,6 @@ async def startup(self) -> None: self.logger.warning(msg) async def execute(self) -> None: - """The work done in each iteration of the loop.""" if self.options.delete_lab or await self._client.is_lab_stopped(): self._image = None if not await self.spawn_lab(): @@ -171,7 +170,7 @@ async def execute(self) -> None: await self.delete_lab() async def execution_idle(self) -> bool: - """Executed between each unit of work execution. + """Pause between each unit of work execution. This is not used directly by `NubladoBusiness`. It should be called by subclasses in `execute_code` in between each block of code that is @@ -184,7 +183,6 @@ async def shutdown(self) -> None: await self.delete_lab() async def idle(self) -> None: - """The idle pause at the end of each loop.""" if self.options.jitter: self.logger.info("Idling...") with self.timings.start("idle"): diff --git a/src/monkeyflocker/cli.py b/src/monkeyflocker/cli.py index 7e5b6aea..44c4726a 100644 --- a/src/monkeyflocker/cli.py +++ b/src/monkeyflocker/cli.py @@ -13,10 +13,7 @@ @click.group(context_settings={"help_option_names": ["-h", "--help"]}) @click.version_option(message="%(version)s") def main() -> None: - """monkeyflocker main. - - Command-line interface to manage mobu monkeys. - """ + """Command-line interface to manage mobu monkeys.""" pass From 1f84823bddd3de2bdac1bfd5b2b39a2c67f0aaab Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 10 May 2023 08:27:13 -0700 Subject: [PATCH 2/5] Allow configuration of the path prefix Follow our current standard for Safir apps and allow configuration of the prefix for all routes. --- changelog.d/20230510_082737_rra_DM_38425.md | 3 +++ src/mobu/config.py | 6 ++++++ src/mobu/main.py | 10 +++++----- 3 files changed, 14 insertions(+), 5 deletions(-) create mode 100644 changelog.d/20230510_082737_rra_DM_38425.md diff --git a/changelog.d/20230510_082737_rra_DM_38425.md b/changelog.d/20230510_082737_rra_DM_38425.md new file mode 100644 index 00000000..df828e36 --- /dev/null +++ b/changelog.d/20230510_082737_rra_DM_38425.md @@ -0,0 +1,3 @@ +### New features + +- The prefix for mobu routes (`/mobu` by default) can now be configured with `SAFIR_PATH_PREFIX`. diff --git a/src/mobu/config.py b/src/mobu/config.py index caed33a6..5f48500a 100644 --- a/src/mobu/config.py +++ b/src/mobu/config.py @@ -104,6 +104,12 @@ class Configuration(BaseSettings): env="SAFIR_NAME", ) + path_prefix: str = Field( + "/mobu", + title="URL prefix for application API", + env="SAFIR_PATH_PREFIX", + ) + profile: Profile = Field( Profile.development, title="Application logging profile", diff --git a/src/mobu/main.py b/src/mobu/main.py index 21a8316f..006e8076 100644 --- a/src/mobu/main.py +++ b/src/mobu/main.py @@ -28,7 +28,7 @@ configure_logging( - profile=config.profile, log_level=config.log_level, name="mobu" + name="mobu", profile=config.profile, log_level=config.log_level ) if config.profile == Profile.production: configure_uvicorn_logging(config.log_level) @@ -37,15 +37,15 @@ title="mobu", description=metadata("mobu")["Summary"], version=version("mobu"), - openapi_url=f"/{config.name}/openapi.json", - docs_url=f"/{config.name}/docs", - redoc_url=f"/{config.name}/redoc", + openapi_url=f"{config.path_prefix}/openapi.json", + docs_url=f"{config.path_prefix}/docs", + redoc_url=f"{config.path_prefix}/redoc", ) """The main FastAPI application for mobu.""" # Attach the routers. app.include_router(internal_router) -app.include_router(external_router, prefix=f"/{config.name}") +app.include_router(external_router, prefix=config.path_prefix) # Add middleware. app.add_middleware(XForwardedMiddleware) From 90d127ac732352656450b3b1917a45a1acee30e9 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 10 May 2023 08:34:23 -0700 Subject: [PATCH 3/5] Enable Slack reporting of uncaught exceptions Turn on the standard Safir machinery for reporting uncaught route handler exceptions to Slack. --- changelog.d/20230510_083114_rra_DM_38425.md | 3 +++ src/mobu/handlers/external.py | 3 ++- src/mobu/handlers/internal.py | 3 ++- src/mobu/main.py | 8 ++++++++ 4 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 changelog.d/20230510_083114_rra_DM_38425.md diff --git a/changelog.d/20230510_083114_rra_DM_38425.md b/changelog.d/20230510_083114_rra_DM_38425.md new file mode 100644 index 00000000..d7435cba --- /dev/null +++ b/changelog.d/20230510_083114_rra_DM_38425.md @@ -0,0 +1,3 @@ +### New features + +- Uncaught exceptions from mobu's route handlers are now also reported to Slack. diff --git a/src/mobu/handlers/external.py b/src/mobu/handlers/external.py index a61805b7..abf67e59 100644 --- a/src/mobu/handlers/external.py +++ b/src/mobu/handlers/external.py @@ -8,6 +8,7 @@ from safir.datetime import current_datetime from safir.metadata import get_metadata from safir.models import ErrorModel +from safir.slack.webhook import SlackRouteErrorHandler from ..config import config from ..dependencies.context import RequestContext, context_dependency @@ -16,7 +17,7 @@ from ..models.monkey import MonkeyData from ..models.solitary import SolitaryConfig, SolitaryResult -external_router = APIRouter() +external_router = APIRouter(route_class=SlackRouteErrorHandler) """FastAPI router for all external handlers.""" __all__ = ["external_router"] diff --git a/src/mobu/handlers/internal.py b/src/mobu/handlers/internal.py index 13caae3e..e9872c4a 100644 --- a/src/mobu/handlers/internal.py +++ b/src/mobu/handlers/internal.py @@ -9,10 +9,11 @@ from fastapi import APIRouter from safir.metadata import Metadata, get_metadata +from safir.slack.webhook import SlackRouteErrorHandler from ..config import config -internal_router = APIRouter() +internal_router = APIRouter(route_class=SlackRouteErrorHandler) """FastAPI router for all internal handlers.""" __all__ = ["internal_router"] diff --git a/src/mobu/main.py b/src/mobu/main.py index 006e8076..f40441cc 100644 --- a/src/mobu/main.py +++ b/src/mobu/main.py @@ -10,11 +10,13 @@ import asyncio from importlib.metadata import metadata, version +import structlog from fastapi import FastAPI, Request, status from fastapi.responses import JSONResponse from safir.logging import Profile, configure_logging, configure_uvicorn_logging from safir.middleware.x_forwarded import XForwardedMiddleware from safir.models import ErrorLocation +from safir.slack.webhook import SlackRouteErrorHandler from .asyncio import schedule_periodic from .config import config @@ -50,6 +52,12 @@ # Add middleware. app.add_middleware(XForwardedMiddleware) +# Enable Slack alerting for uncaught exceptions. +if config.alert_hook: + logger = structlog.get_logger("mobu") + SlackRouteErrorHandler.initialize(config.alert_hook, "mobu", logger) + logger.debug("Initialized Slack webhook") + @app.on_event("startup") async def startup_event() -> None: From 32805ccff6b9b6435ecf3253b614076db34a9c4f Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 10 May 2023 08:39:38 -0700 Subject: [PATCH 4/5] Fix the Makefile run target The Makefile run target was still using the aiohttp command. Use tox run -e run instead. --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index cdfd6dee..959ddfee 100644 --- a/Makefile +++ b/Makefile @@ -25,4 +25,4 @@ update: update-deps init .PHONY: run run: - adev runserver --app-factory create_app src/mobu/app.py + tox run -e run From f0e97963506a4cd41ca19059de874b629f15fffc Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 10 May 2023 09:38:07 -0700 Subject: [PATCH 5/5] Use the Safir ClientRequestError framework Rather than using separate exception handlers for the exceptions for unknown flocks and unknown monkeys, define those exceptions using the Safir ClientRequestError framework and use the generic exception handler from Safir. Rename those exceptions to end in Error instead of Exception, following our normal naming conventions. --- src/mobu/exceptions.py | 47 ++++++++++++++++++++++-------------- src/mobu/main.py | 47 +++++------------------------------- src/mobu/services/flock.py | 22 ++++++++++++++--- src/mobu/services/manager.py | 13 +++++++--- 4 files changed, 63 insertions(+), 66 deletions(-) diff --git a/src/mobu/exceptions.py b/src/mobu/exceptions.py index e4b1439e..8731238c 100644 --- a/src/mobu/exceptions.py +++ b/src/mobu/exceptions.py @@ -6,8 +6,11 @@ from datetime import datetime from typing import Optional, Self +from fastapi import status from httpx_ws import HTTPXWSException, WebSocketDisconnect from safir.datetime import format_datetime_for_logging +from safir.fastapi import ClientRequestError +from safir.models import ErrorLocation from safir.slack.blockkit import ( SlackBaseBlock, SlackBaseField, @@ -25,13 +28,13 @@ __all__ = [ "CachemachineError", "CodeExecutionError", - "FlockNotFoundException", + "FlockNotFoundError", "GafaelfawrWebError", "JupyterTimeoutError", "JupyterWebError", "MobuSlackException", "MobuSlackWebException", - "MonkeyNotFoundException", + "MonkeyNotFoundError", "TAPClientError", ] @@ -63,6 +66,30 @@ class GafaelfawrWebError(SlackWebException): """An API call to Gafaelfawr failed.""" +class FlockNotFoundError(ClientRequestError): + """The named flock was not found.""" + + error = "flock_not_found" + status_code = status.HTTP_404_NOT_FOUND + + def __init__(self, flock: str) -> None: + self.flock = flock + msg = f"Flock {flock} not found" + super().__init__(msg, ErrorLocation.path, ["flock"]) + + +class MonkeyNotFoundError(ClientRequestError): + """The named monkey was not found.""" + + error = "monkey_not_found" + status_code = status.HTTP_404_NOT_FOUND + + def __init__(self, monkey: str) -> None: + self.monkey = monkey + msg = f"Monkey {monkey} not found" + super().__init__(msg, ErrorLocation.path, ["monkey"]) + + class MobuSlackException(SlackException): """Represents an exception that can be reported to Slack. @@ -159,22 +186,6 @@ class MobuSlackWebException(SlackWebException, MobuSlackException): """ -class FlockNotFoundException(Exception): - """The named flock was not found.""" - - def __init__(self, flock: str) -> None: - self.flock = flock - super().__init__(f"Flock {flock} not found") - - -class MonkeyNotFoundException(Exception): - """The named monkey was not found.""" - - def __init__(self, monkey: str) -> None: - self.monkey = monkey - super().__init__(f"Monkey {monkey} not found") - - class NotebookRepositoryError(MobuSlackException): """The repository containing notebooks to run is not valid.""" diff --git a/src/mobu/main.py b/src/mobu/main.py index f40441cc..d76de3f9 100644 --- a/src/mobu/main.py +++ b/src/mobu/main.py @@ -11,22 +11,20 @@ from importlib.metadata import metadata, version import structlog -from fastapi import FastAPI, Request, status -from fastapi.responses import JSONResponse +from fastapi import FastAPI +from safir.fastapi import ClientRequestError, client_request_error_handler from safir.logging import Profile, configure_logging, configure_uvicorn_logging from safir.middleware.x_forwarded import XForwardedMiddleware -from safir.models import ErrorLocation from safir.slack.webhook import SlackRouteErrorHandler from .asyncio import schedule_periodic from .config import config from .dependencies.context import context_dependency -from .exceptions import FlockNotFoundException, MonkeyNotFoundException from .handlers.external import external_router from .handlers.internal import internal_router from .status import post_status -__all__ = ["app", "config"] +__all__ = ["app"] configure_logging( @@ -58,6 +56,9 @@ SlackRouteErrorHandler.initialize(config.alert_hook, "mobu", logger) logger.debug("Initialized Slack webhook") +# Enable the generic exception handler for client errors. +app.exception_handler(ClientRequestError)(client_request_error_handler) + @app.on_event("startup") async def startup_event() -> None: @@ -78,39 +79,3 @@ async def shutdown_event() -> None: await app.state.periodic_status except asyncio.CancelledError: pass - - -@app.exception_handler(FlockNotFoundException) -async def flock_not_found_exception_handler( - request: Request, exc: FlockNotFoundException -) -> JSONResponse: - return JSONResponse( - status_code=status.HTTP_404_NOT_FOUND, - content={ - "detail": [ - { - "loc": [ErrorLocation.path, "flock"], - "msg": f"Flock for {exc.flock} not found", - "type": "flock_not_found", - } - ] - }, - ) - - -@app.exception_handler(MonkeyNotFoundException) -async def monkey_not_found_exception_handler( - request: Request, exc: MonkeyNotFoundException -) -> JSONResponse: - return JSONResponse( - status_code=status.HTTP_404_NOT_FOUND, - content={ - "detail": [ - { - "loc": [ErrorLocation.path, "monkey"], - "msg": f"Monkey for {exc.monkey} not found", - "type": "monkey_not_found", - } - ] - }, - ) diff --git a/src/mobu/services/flock.py b/src/mobu/services/flock.py index 2e85ebc4..7f04f233 100644 --- a/src/mobu/services/flock.py +++ b/src/mobu/services/flock.py @@ -12,7 +12,7 @@ from safir.datetime import current_datetime from structlog.stdlib import BoundLogger -from ..exceptions import MonkeyNotFoundException +from ..exceptions import MonkeyNotFoundError from ..models.flock import FlockConfig, FlockData, FlockSummary from ..models.user import AuthenticatedUser, User, UserSpec from .monkey import Monkey @@ -60,10 +60,26 @@ def dump(self) -> FlockData: ) def get_monkey(self, name: str) -> Monkey: - """Retrieve a given monkey by name.""" + """Retrieve a given monkey by name. + + Parameters + ---------- + name + Name of monkey to return. + + Returns + ------- + Monkey + Requested monkey. + + Raises + ------ + MonkeyNotFoundError + Raised if no monkey was found with that name. + """ monkey = self._monkeys.get(name) if not monkey: - raise MonkeyNotFoundException(name) + raise MonkeyNotFoundError(name) return monkey def list_monkeys(self) -> list[str]: diff --git a/src/mobu/services/manager.py b/src/mobu/services/manager.py index 2ee871c2..7d722ef4 100644 --- a/src/mobu/services/manager.py +++ b/src/mobu/services/manager.py @@ -10,7 +10,7 @@ from structlog.stdlib import BoundLogger from ..config import config -from ..exceptions import FlockNotFoundException +from ..exceptions import FlockNotFoundError from ..models.flock import FlockConfig, FlockSummary from .flock import Flock @@ -98,12 +98,12 @@ def get_flock(self, name: str) -> Flock: Raises ------ - FlockNotFoundException + FlockNotFoundError Raised if no flock was found with that name. """ flock = self._flocks.get(name) if flock is None: - raise FlockNotFoundException(name) + raise FlockNotFoundError(name) return flock def list_flocks(self) -> list[str]: @@ -133,9 +133,14 @@ async def stop_flock(self, name: str) -> None: ---------- name Name of flock to stop. + + Raises + ------ + FlockNotFoundError + Raised if no flock was found with that name. """ flock = self._flocks.get(name) if flock is None: - raise FlockNotFoundException(name) + raise FlockNotFoundError(name) del self._flocks[name] await flock.stop()