From 537c57d46533157c35589a1c26e577bedd7ee2e1 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 2 Jul 2025 15:57:49 +0100 Subject: [PATCH 1/4] Remove the file_manager module `file_manager` was a previous attempt to handle actions that created files. It is retired in favour of the `Blob` class. I'm not aware of any examples of `file_manager` being used "in the wild" and it does not do anything that `Blob` can't do. We are therefore getting rid of it as vestigial code. --- src/labthings_fastapi/actions/__init__.py | 55 ---------------- src/labthings_fastapi/descriptors/action.py | 26 +++----- src/labthings_fastapi/file_manager.py | 72 --------------------- tests/test_dependencies.py | 15 ----- tests/test_dependencies_2.py | 15 ----- tests/test_temp_files.py | 45 ------------- 6 files changed, 10 insertions(+), 218 deletions(-) delete mode 100644 src/labthings_fastapi/file_manager.py delete mode 100644 tests/test_temp_files.py 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_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() From ee95b1f843e24d0a262890a605e1cb2f51f63d07 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 2 Jul 2025 22:40:11 +0100 Subject: [PATCH 2/4] Check we can get a list of invocations This is a bonus extra test, to stop the coverage diff being negative after deleting FileManager. --- tests/test_action_manager.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index 84096086..2fd0c47b 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): @@ -31,7 +32,7 @@ def run(payload=None): return run -def test_expiry(): +def test_action_expires(): with TestClient(server.app) as client: before_value = client.get("/thing/counter").json() r = client.post("/thing/increment_counter") @@ -42,5 +43,17 @@ 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(): + 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] From b21c7f1920d75efaa808c3dc508b286655be3590 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 2 Jul 2025 22:51:01 +0100 Subject: [PATCH 3/4] Exclude all type checking imports from coverage reports --- .coveragerc | 5 +++++ 1 file changed, 5 insertions(+) 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: From 59c8fe0f8355957defe0f01ff75b462d9f0b43d9 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 3 Jul 2025 01:23:52 +0100 Subject: [PATCH 4/4] Add a docstring to the new test and delete redundant function --- tests/test_action_manager.py | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index 2fd0c47b..4ae2ab4f 100644 --- a/tests/test_action_manager.py +++ b/tests/test_action_manager.py @@ -23,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_action_expires(): + 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") @@ -50,6 +54,13 @@ def test_action_expires(): 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())