diff --git a/.coveragerc b/.coveragerc index 7280bd89..de80c527 100644 --- a/.coveragerc +++ b/.coveragerc @@ -3,3 +3,8 @@ concurrency = multiprocessing, thread parallel = true sigterm = true omit = tests/**/*.py, docs/**/*.py + +[report] +exclude_lines = + pragma: no cover + if TYPE_CHECKING: diff --git a/src/labthings_fastapi/actions/__init__.py b/src/labthings_fastapi/actions/__init__.py index ec75a1c8..7532a87e 100644 --- a/src/labthings_fastapi/actions/__init__.py +++ b/src/labthings_fastapi/actions/__init__.py @@ -8,13 +8,11 @@ from typing import TYPE_CHECKING import weakref from fastapi import FastAPI, HTTPException, Request -from fastapi.responses import FileResponse from pydantic import BaseModel from ..utilities import model_to_dict from ..utilities.introspection import EmptyInput from ..thing_description.model import LinkElement -from ..file_manager import FileManager from .invocation_model import InvocationModel, InvocationStatus from ..dependencies.invocation import ( CancelHook, @@ -69,10 +67,6 @@ def __init__( self.retention_time = action.retention_time self.expiry_time: Optional[datetime.datetime] = None - # This is added post-hoc by the FastAPI endpoint, in - # `ActionDescriptor.add_to_fastapi` - self._file_manager: Optional[FileManager] = None - # Private state properties self._status_lock = Lock() # This Lock protects properties below self._status: InvocationStatus = InvocationStatus.PENDING # Task status @@ -148,8 +142,6 @@ def response(self, request: Optional[Request] = None): LinkElement(rel="self", href=href), LinkElement(rel="output", href=href + "/output"), ] - if self._file_manager: - links += self._file_manager.links(href) return self.action.invocation_model( status=self.status, id=self.id, @@ -413,50 +405,3 @@ def delete_invocation(id: uuid.UUID) -> None: ), ) invocation.cancel() - - @app.get( - ACTION_INVOCATIONS_PATH + "/{id}/files", - responses={ - 404: {"description": "Invocation ID not found"}, - 503: {"description": "No files are available for this invocation"}, - }, - ) - def action_invocation_files(id: uuid.UUID) -> list[str]: - with self._invocations_lock: - try: - invocation: Any = self._invocations[id] - except KeyError: - raise HTTPException( - status_code=404, - detail="No action invocation found with ID {id}", - ) - if not invocation._file_manager: - raise HTTPException( - status_code=503, - detail="No files are available for this invocation", - ) - return invocation._file_manager.filenames - - @app.get( - ACTION_INVOCATIONS_PATH + "/{id}/files/{filename}", - response_class=FileResponse, - responses={ - 404: {"description": "Invocation ID not found, or file not found"}, - 503: {"description": "No files are available for this invocation"}, - }, - ) - def action_invocation_file(id: uuid.UUID, filename: str): - with self._invocations_lock: - try: - invocation: Any = self._invocations[id] - except KeyError: - raise HTTPException( - status_code=404, - detail="No action invocation found with ID {id}", - ) - if not invocation._file_manager: - raise HTTPException( - status_code=503, - detail="No files are available for this invocation", - ) - return FileResponse(invocation._file_manager.path(filename)) diff --git a/src/labthings_fastapi/descriptors/action.py b/src/labthings_fastapi/descriptors/action.py index 3ddde574..d2bf1084 100644 --- a/src/labthings_fastapi/descriptors/action.py +++ b/src/labthings_fastapi/descriptors/action.py @@ -186,22 +186,16 @@ def start_action( background_tasks: BackgroundTasks, **dependencies, ): - try: - action = action_manager.invoke_action( - action=self, - thing=thing, - input=body, - dependencies=dependencies, - id=id, - cancel_hook=cancel_hook, - ) - background_tasks.add_task(action_manager.expire_invocations) - return action.response(request=request) - finally: - try: - action._file_manager = request.state.file_manager - except AttributeError: - pass # This probably means there was no FileManager created. + action = action_manager.invoke_action( + action=self, + thing=thing, + input=body, + dependencies=dependencies, + id=id, + cancel_hook=cancel_hook, + ) + background_tasks.add_task(action_manager.expire_invocations) + return action.response(request=request) if issubclass(self.input_model, EmptyInput): annotation = Body(default_factory=StrictEmptyInput) diff --git a/src/labthings_fastapi/file_manager.py b/src/labthings_fastapi/file_manager.py deleted file mode 100644 index 38b66971..00000000 --- a/src/labthings_fastapi/file_manager.py +++ /dev/null @@ -1,72 +0,0 @@ -"""Manage files created by Actions - -**This module is deprecated in favour of `labthings_fastapi.outputs.blob`** - -Simple actions return everything you need to know about them in their return value, -which can be serialised to JSON. More complicated actions might need to return -more complicated responses, for example files. - -The FileManager class is responsible for managing files created by actions. It will -handle finding a temporary storage location, and making it possible to retrieve the -files via the Invocation object. -""" - -from __future__ import annotations -from tempfile import TemporaryDirectory -from typing import Annotated, Sequence, Optional -from warnings import warn - -from fastapi import Depends, Request - -from .thing_description.model import LinkElement -import os - -from .dependencies.invocation import InvocationID - - -class FileManager: - """Manage files created by Actions""" - - __globals__ = globals() # "bake in" globals so dependency injection works - - def __init__(self, invocation_id: InvocationID, request: Request): - warn( - "FileManager is deprecated in favour of labthings_fastapi.outputs.blob", - DeprecationWarning, - ) - self.invocation_id = invocation_id - self._links: set[tuple[str, str]] = set() - self._dir = TemporaryDirectory(prefix=f"labthings-{self.invocation_id}-") - request.state.file_manager = self - # The request state will be used to hold onto the FileManager after - # the action has finished - - @property - def directory(self) -> str: - """Return the temporary directory for this invocation""" - return self._dir.name - - @property - def filenames(self) -> list[str]: - """A list of files currently being managed by this FileManager""" - return os.listdir(self.directory) - - def add_link(self, rel: str, filename: str) -> None: - """Make a file show up in the links of the Invocation""" - self._links.add((rel, filename)) - - def path(self, filename: str, rel: Optional[str] = None) -> str: - """Return the path to a file""" - if rel is not None: - self.add_link(rel, filename) - return os.path.join(self.directory, filename) - - def links(self, prefix: str) -> Sequence[LinkElement]: - """Generate links to the files managed by this FileManager""" - links = [LinkElement(rel="files", href=prefix + "/files")] - for rel, filename in self._links: - links.append(LinkElement(rel=rel, href=prefix + "/files/" + filename)) - return links - - -FileManagerDep = Annotated[FileManager, Depends()] diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index 84096086..4ae2ab4f 100644 --- a/tests/test_action_manager.py +++ b/tests/test_action_manager.py @@ -4,6 +4,7 @@ from temp_client import poll_task import time import labthings_fastapi as lt +from labthings_fastapi.actions import ACTION_INVOCATIONS_PATH class TestThing(lt.Thing): @@ -22,16 +23,20 @@ def increment_counter(self): server.add_thing(thing, "/thing") -def action_partial(client: TestClient, url: str): - def run(payload=None): - r = client.post(url, json=payload) - assert r.status_code in (200, 201) - return poll_task(client, r.json()) +def test_action_expires(): + """Check the action is removed from the server - return run + We've set the retention period to be very short, so the action + should not be retrievable after some time has elapsed. + This test checks that actions do indeed get removed. -def test_expiry(): + Note that the code that expires actions runs whenever a new + action is started. That's why we need to invoke the action twice: + the second invocation runs the code that deletes the first one. + This behaviour might change in the future, making the second run + unnecessary. + """ with TestClient(server.app) as client: before_value = client.get("/thing/counter").json() r = client.post("/thing/increment_counter") @@ -42,5 +47,24 @@ def test_expiry(): after_value = client.get("/thing/counter").json() assert after_value == before_value + 2 invocation["status"] = "running" # Force an extra poll + # When the second action runs, the first one should expire + # so polling it again should give a 404. with pytest.raises(httpx.HTTPStatusError): poll_task(client, invocation) + + +def test_actions_list(): + """Check that the /action_invocations/ path works. + + The /action_invocations/ path should return a list of invocation + objects (a representation of each action that's been run recently). + + It's implemented in `ActionManager.list_all_invocations`. + """ + with TestClient(server.app) as client: + r = client.post("/thing/increment_counter") + invocation = poll_task(client, r.json()) + r2 = client.get(ACTION_INVOCATIONS_PATH) + r2.raise_for_status() + invocations = r2.json() + assert invocations == [invocation] diff --git a/tests/test_dependencies.py b/tests/test_dependencies.py index 82052840..2d6f4689 100644 --- a/tests/test_dependencies.py +++ b/tests/test_dependencies.py @@ -6,7 +6,6 @@ from fastapi import Depends, FastAPI, Request from labthings_fastapi.deps import InvocationID -from labthings_fastapi.file_manager import FileManagerDep from fastapi.testclient import TestClient from module_with_deps import FancyIDDep @@ -55,17 +54,3 @@ def endpoint(id: DepClass = Depends()) -> bool: assert r.status_code == 200 invocation = r.json() assert invocation is True - - -def test_file_manager(): - app = FastAPI() - - @app.post("/invoke_with_file") - def invoke_with_file( - file_manager: FileManagerDep, - ) -> dict[str, str]: - return {"directory": str(file_manager.directory)} - - with TestClient(app) as client: - r = client.post("/invoke_with_file") - assert r.status_code == 200 diff --git a/tests/test_dependencies_2.py b/tests/test_dependencies_2.py index 50251f9d..cf5a2a33 100644 --- a/tests/test_dependencies_2.py +++ b/tests/test_dependencies_2.py @@ -22,7 +22,6 @@ from fastapi.testclient import TestClient from module_with_deps import FancyIDDep, FancyID, ClassDependsOnFancyID import labthings_fastapi as lt -from labthings_fastapi.file_manager import FileManager def test_dep_from_module(): @@ -147,17 +146,3 @@ def endpoint(id: lt.deps.InvocationID) -> bool: with TestClient(app) as client: r = client.post("/endpoint") assert r.status_code == 200 - - -def test_filemanager_dep(): - """Test out our FileManager class as a dependency""" - app = FastAPI() - - @app.post("/endpoint") - def endpoint(fm: Annotated[FileManager, Depends()]) -> str: - return f"Saving to {fm.directory}" - - with TestClient(app) as client: - r = client.post("/endpoint") - assert r.status_code == 200 - assert r.json().startswith("Saving to ") diff --git a/tests/test_temp_files.py b/tests/test_temp_files.py deleted file mode 100644 index 8c8cb795..00000000 --- a/tests/test_temp_files.py +++ /dev/null @@ -1,45 +0,0 @@ -import labthings_fastapi as lt -from labthings_fastapi.file_manager import FileManagerDep -from fastapi.testclient import TestClient -from temp_client import poll_task, get_link - - -class FileThing(lt.Thing): - @lt.thing_action - def write_message_file( - self, - file_manager: FileManagerDep, - message: str = "Hello World", - ) -> dict[str, str]: - """Write a message to a file.""" - # We should be able to call actions as normal Python functions - with open(file_manager.path("message.txt", rel="message_file"), "w") as f: - f.write(message) - with open(file_manager.path("message2.txt"), "w") as f: - f.write(message) - return {"filename": "message.txt"} - - -thing = FileThing() -server = lt.ThingServer() -server.add_thing(thing, "/thing") - - -def test_td_validates(): - thing.validate_thing_description() - - -def test_action_output(): - client = TestClient(server.app) - r = client.post("/thing/write_message_file", json={}) - invocation = poll_task(client, r.json()) - assert invocation["status"] == "completed" - assert invocation["output"] == {"filename": "message.txt"} - r = client.get(get_link(invocation, "message_file")["href"]) - assert r.status_code == 200 - assert r.text == "Hello World" - - -if __name__ == "__main__": - test_td_validates() - test_action_output()