From b09214b046c3f565d596f3df3270f5fea8209ad5 Mon Sep 17 00:00:00 2001 From: AcquaDiGiorgio <64639359+AcquaDiGiorgio@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:39:20 +0100 Subject: [PATCH 1/3] docs: FutureClient developement --- .../DevelopingFutureClients/DIRACClient.py | 9 + .../DevelopingFutureClients/DIRACClientNew.py | 15 + .../DIRACFutureClient.py | 19 ++ .../DevelopingFutureClients/DIRACHandler.py | 26 ++ .../DevelopingFutureClients/DiracXLogic.py | 23 ++ .../DevelopingFutureClients/DiracXRouter.py | 45 +++ .../DevelopingFutureClients/index.rst | 266 ++++++++++++++++++ .../DIRACToDiracXBridge/index.rst | 11 + docs/source/DeveloperGuide/index.rst | 1 + 9 files changed, 415 insertions(+) create mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClient.py create mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClientNew.py create mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACFutureClient.py create mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACHandler.py create mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXLogic.py create mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXRouter.py create mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/index.rst create mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/index.rst diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClient.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClient.py new file mode 100644 index 00000000000..3b37067e823 --- /dev/null +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClient.py @@ -0,0 +1,9 @@ +# DIRAC/WorkloadManagementSystem/Client/JobAnnotatorClient.py +from DIRAC.Core.Base.Client import Client, createClient + + +@createClient("WorkloadManagement/JobAnnotator") +class JobAnnotatorClient(Client): + def __init__(self, url=None, **kwargs): + super().__init__(**kwargs) + self.serverURL = url or "WorkloadManagement/JobAnnotator" diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClientNew.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClientNew.py new file mode 100644 index 00000000000..6f07442dc68 --- /dev/null +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClientNew.py @@ -0,0 +1,15 @@ +# DIRAC/WorkloadManagementSystem/Client/JobAnnotatorClient.py +from DIRAC.WorkloadManagementSystem.FutureClient.JobAnnotatorClient import ( + JobAnnotatorClient as futureJobAnnotatorClient, +) + +from DIRAC.Core.Base.Client import Client, createClient + + +@createClient("WorkloadManagement/JobAnnotator") +class JobAnnotatorClient(Client): + diracxClient = futureJobAnnotatorClient # <-- the only change + + def __init__(self, url=None, **kwargs): + super().__init__(**kwargs) + self.serverURL = url or "WorkloadManagement/JobAnnotator" diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACFutureClient.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACFutureClient.py new file mode 100644 index 00000000000..fd37416a74c --- /dev/null +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACFutureClient.py @@ -0,0 +1,19 @@ +# DIRAC/WorkloadManagementSystem/FutureClient/JobAnnotatorClient.py +from __future__ import annotations + +from DIRAC.Core.Security.DiracX import DiracXClient, FutureClient, addRPCStub +from DIRAC.Core.Utilities.ReturnValues import convertToReturnValue + + +class JobAnnotatorClient(FutureClient): + @addRPCStub + @convertToReturnValue + def addAnnotation(self, jobID: str | int, annotation: str): + with DiracXClient() as api: + # Legacy accepts str or int, DiracX expects an int: + api.jobs.add_annotation({int(jobID): annotation}) + + @convertToReturnValue + def getAnnotations(self, jobID: str | int): + with DiracXClient() as api: + return api.jobs.get_annotations(int(jobID)) diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACHandler.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACHandler.py new file mode 100644 index 00000000000..6b07c6f8a75 --- /dev/null +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACHandler.py @@ -0,0 +1,26 @@ +# DIRAC/WorkloadManagementSystem/Service/JobAnnotatorHandler.py +from DIRAC import S_ERROR, S_OK +from DIRAC.Core.DISET.RequestHandler import RequestHandler +from DIRAC.Core.Utilities.ObjectLoader import ObjectLoader + + +class JobAnnotatorHandler(RequestHandler): + @classmethod + def initializeHandler(cls, infoDict): + result = ObjectLoader().loadObject("WorkloadManagementSystem.DB.JobAnnotatorDB", "JobAnnotatorDB") + if not result["OK"]: + return result + cls.annotatorDB = result["Value"]() + return S_OK() + + types_addAnnotation = [[str, int], str] + + def export_addAnnotation(self, jobID, annotation): + """Attach an annotation to a job.""" + return self.annotatorDB.addAnnotation(int(jobID), annotation) + + types_getAnnotations = [[str, int]] + + def export_getAnnotations(self, jobID): + """Return the list of annotations of a job.""" + return self.annotatorDB.getAnnotations(int(jobID)) diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXLogic.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXLogic.py new file mode 100644 index 00000000000..2ad15089e06 --- /dev/null +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXLogic.py @@ -0,0 +1,23 @@ +# diracx-logic/src/diracx/logic/jobs/annotations.py +from __future__ import annotations + +from diracx.core.exceptions import NotReadyError +from diracx.db.sql import JobDB + + +async def add_annotations(annotations: dict[int, str], job_db: JobDB) -> None: + """Attach an annotation to each of the given jobs.""" + # Extremely efficient bulk operation. O(-n) probably + for job_id, annotation in annotations.items(): + if not await job_db.exists(job_id): + raise NotReadyError("I wasn't prepared, you took me by surprise") + + await job_db.insert_annotation(job_id, annotation) + + +async def get_annotations(job_id: int, job_db: JobDB) -> list[str]: + """Return all the annotations of a job.""" + if not await job_db.exists(job_id): + raise NotReadyError("AHHHHH") + + return await job_db.fetch_annotations(job_id) diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXRouter.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXRouter.py new file mode 100644 index 00000000000..c2dc537f2df --- /dev/null +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXRouter.py @@ -0,0 +1,45 @@ +# diracx-routers/src/diracx/routers/jobs/annotations.py +from __future__ import annotations + +from http import HTTPStatus +from typing import Annotated + +from diracx.core.exceptions import NotReadyError +from diracx.db.sql import JobDB +from diracx.logic.jobs import add_annotations as add_annotations_bl +from diracx.logic.jobs import get_annotations as get_annotations_bl +from fastapi import Body, HTTPException + +from ..fastapi_classes import DiracxRouter + +router = DiracxRouter() + + +@router.post("/annotations", status_code=HTTPStatus.NO_CONTENT) +async def add_annotation( + annotations: Annotated[dict[int, str], Body()], + job_db: JobDB, +): + """Attach an annotation to one or more jobs. What's an annotation? Who knows""" + + # MAGIC LINES PROBABLY WRITTEN AND REVIWED BY CLAUDE 🦀 + + try: + await add_annotations_bl(annotations, job_db) + except NotReadyError as e: + raise HTTPException(status_code=HTTPStatus.PAYMENT_REQUIRED, detail=str(e)) from e + + +@router.get("/{job_id}/annotations") +async def get_annotations( + job_id: int, + job_db: JobDB, +) -> list[str]: + """Return the annotations of a job.""" + + # MORE MAGIC LINES. DID YOU EXPECT SOMETHING DIFFERENT? + + try: + return await get_annotations_bl(job_id, job_db) + except NotReadyError as e: + raise HTTPException(status_code=HTTPStatus.PAYMENT_REQUIRED, detail=str(e)) from e diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/index.rst b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/index.rst new file mode 100644 index 00000000000..b76c8cda7b9 --- /dev/null +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/index.rst @@ -0,0 +1,266 @@ +====================================== +Developing Future Clients +====================================== + +This is a step by step guide to develop proper DIRAC *FutureClients* for a +seemless DIRAC-DiracX compatibility. + +A ``FutureClient`` is a replacement of a legacy DIRAC ``Client`` that is capable of +talking to DiracX in a transparent way for the user. + +.. contents:: + :local: + :depth: 1 + + +Overview +======== + +DIRAC and DiracX access points are organized differently: + +* **DIRAC** exposes a service through a **Handler** (the implementation) and a + **Client** (the access point). +* **DiracX** has no handlers. It has an **API** that uses the ``logic`` through + the ``routers``. + +The call chain of a migrated service is: + +.. code-block:: text + + Client (legacy) -> FutureClient -> DiracX API -> DiracX router -> DiracX logic + +For ease of understanding, a simple made up service will be used as examples: + +.. code-block:: text + + DIRAC + └── src/DIRAC/WorkloadManagementSystem/ + ├── Service/JobAnnotatorHandler.py + ├── Client/JobAnnotatorClient.py + └── FutureClient/JobAnnotatorClient.py + + diracx + ├── diracx-routers/src/diracx/routers/jobs/annotations.py # API: HTTP endpoints + └── diracx-logic/src/diracx/logic/jobs/annotations.py # API: business logic + + +Step 1: Understand requirements +=============================== + +Before writing any code, find out exactly what the legacy service does. + +DIRAC exposes a service through two parts: + +* The **Handler**, located in ``Service/Handler.py``, which contains the + real implementation. Each remotely callable method is called + ``export_`` and its accepted argument types are declared in + ``types_`` right above. +* The **Client**, located in ``Client/Client.py``, is almost always + empty and only has a decorator that points to the Handler. + +In this example, ``WorkloadManagement/JobAnnotator`` points to +``WorkloadManagementSystem/Service/JobAnnotatorHandler.py``. + +The made up legacy Handler: + +.. literalinclude:: DIRACHandler.py + :language: python + :linenos: + +And the legacy Client: + +.. literalinclude:: DIRACClient.py + :language: python + :linenos: + +Write down, for every ``export_*`` method of the Handler, what parameters it +receives and what it returns. + +While developing the ``FutureClient``, the code +might differ a lot, but the inputs and outputs **MUST** stay exactly the same as the +one in the legacy client. + +Step 2: Check current DiracX API +======================================= + +Now check whether DiracX already provides everything you need. Look at the +routers of the DiracX API, in +``diracx-routers/src/diracx/routers//``. A group is built in its +``__init__.py`` by including several routers. For example, the ``jobs`` group +looks like this: + +.. code-block:: python + + # diracx-routers/src/diracx/routers/jobs/__init__.py + # Router definition + router = DiracxRouter() + router.include_router(sandboxes_router) + router.include_router(status_router) + router.include_router(query_router) + router.include_router(submission_router) + +Every endpoint of those routers is available in the DiracX API as +``api..``. For example, a router function: + +.. code-block:: python + + @router.patch("/something") + async def do_something(...): ... + +will be called from DIRAC as: + +.. code-block:: python + + with DiracXClient() as api: + api.jobs.do_something(...) + +Try to map each legacy method to a DiracX operation. The mapping is **NOT** always +one to one: + +* DiracX endpoints are modern and might mix or split DIRAC handler's methods. +* The arguments usually need to be reshaped. For example, there are DiracX endpoints + that take a dictionary keyed by ``jobID``, so many jobs can be updated at once, + something that DIRAC doesn't usually do. +* The returned value can be different and may need to be converted back to the + legacy format. + +Then decide if every legacy method can be mapped or can use a combination of APIs. If so, then **skip Step 3** +and go directly to Step 4. +If something is missing, continue with Step 3. + +For the ``JobAnnotator`` example, since it's made up, there is nothing about it +in any of the existing routers, so we need Step 3. + +Step 3: Develop new DiracX API +===================================== + +The API is split in two layers: + +* **logic** (``diracx-logic``): the business logic, independent of HTTP. +* **routers** (``diracx-routers``): the HTTP endpoints. They check + permissions, call the logic and translate exceptions into HTTP errors. + +3.1 Where does the new code go? +------------------------------- + +In this example the annotations belong to jobs, so the new code goes into the +existing ``jobs`` group of DiracX: + +.. code-block:: text + + diracx/ + ├── diracx-logic/src/diracx/logic/jobs/annotations.py + └── diracx-routers/src/diracx/routers/jobs/annotations.py + +Keep both layers in the same group directory (``jobs``) and with the same file +name (``annotations.py``). + +.. important:: + + ``jobs`` is only the right place **for this example**. For other + developments, the new code might need to go to another existing directory + (such as pilots or auth), or even to a new group directory. + + Keep in mind that the group is also the namespace of the DiracX + client: code placed in ``routers//`` is called from DIRAC as + ``api..``, so choose the group name carefully, + as it becomes part of the public API. + +3.2 Logic +--------- + +.. literalinclude:: DiracXLogic.py + :language: python + :linenos: + + +3.3 Router +---------- + +.. literalinclude:: DiracXRouter.py + :language: python + :linenos: + +Then register the new router in the ``jobs`` group, next to the existing ones: + +.. code-block:: python + + # diracx-routers/src/diracx/routers/jobs/__init__.py + # (other imports) + from .annotations import router as annotations_router + + # Router definition + router = DiracxRouter() + router.include_router(sandboxes_router) + router.include_router(status_router) + router.include_router(query_router) + router.include_router(submission_router) + router.include_router(annotations_router) # <-- new + +Because ``annotations_router`` is included in the ``jobs`` router, its +endpoints end up in the ``jobs`` group of the DiracX API. + +3.4 DiracX API +----------------- + +There is no code to write here. The DiracX API will need to be generated from the routers. + +Once your DiracX change is merged, the new endpoints appear in the +client with the name of the router function, inside the group of the router: + +====================== ====================================== ============================== +Router function Endpoint DiracX API call +====================== ====================================== ============================== +``add_annotation`` ``POST /api/jobs/annotations`` ``api.jobs.add_annotation`` +``get_annotations`` ``GET /api/jobs/{job_id}/annotations`` ``api.jobs.get_annotations`` +====================== ====================================== ============================== + +Step 4: Code the new Future Client in DIRAC +=========================================== + +Create the FutureClient in the ``FutureClient`` directory of the same +system: + +.. code-block:: text + + DIRAC/WorkloadManagementSystem/FutureClient/JobAnnotatorClient.py + +A FutureClient: + +* Inherits from ``FutureClient``. +* Implements **the same methods, with the same arguments** as the legacy + service. +* Calls DiracX through the ``DiracXClient`` context manager. + +.. literalinclude:: DIRACFutureClient.py + :language: python + :linenos: + +Step 5: Change the DIRAC Client configuration +============================================= + +The legacy Client requires just a minimal change in its configuration +for it to see the new changes: + +.. literalinclude:: DIRACClientNew.py + :language: python + :linenos: + +Now the FutureClient is ready to roll into production. Yay :) + +Disabling the DiracX API +--------------------------- + +The DiracX API can be disabled for a given service from the web interface +(the Configuration System), so DIRAC keeps using the legacy Client and Handler +for it. The option is located under ``DiracX/legacyClientEnabled``. + +For this example, to disable the use of the ``FutureClient``, the config would +be as follows: + +.. code-block:: text + + DiracX + └── legacyClientEnabled + └── WorkloadManagement + └── JobAnnotator = yes diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/index.rst b/docs/source/DeveloperGuide/DIRACToDiracXBridge/index.rst new file mode 100644 index 00000000000..28ba685a89d --- /dev/null +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/index.rst @@ -0,0 +1,11 @@ +========================= +Bridging DIRAC and DiracX +========================= + +This section explains how to bridge DIRAC and DiracX while the migration is +still on going and DiracX being actively developed. + +.. toctree:: + :maxdepth: 1 + + DevelopingFutureClients/index diff --git a/docs/source/DeveloperGuide/index.rst b/docs/source/DeveloperGuide/index.rst index 67562c428a1..7b7883a3db3 100644 --- a/docs/source/DeveloperGuide/index.rst +++ b/docs/source/DeveloperGuide/index.rst @@ -37,3 +37,4 @@ For questions, comments, or operational issues, use `GitHub discussions Date: Wed, 4 Nov 2026 18:04:32 +0100 Subject: [PATCH 2/3] refactor: move stripValueIfOK next to addRPCStub stripValueIfOK was only defined in the JobStateUpdateClient FutureClient, but it is the convention for every FutureClient write method. Move it to DIRAC.Core.Security.DiracX so other FutureClients (and the developer guide example) can import it from the same place as addRPCStub. Assisted-by: Claude:claude-opus-5-5 --- .../DevelopingFutureClients/DIRACClientNew.py | 15 ------------ src/DIRAC/Core/Security/DiracX.py | 21 +++++++++++++++++ .../FutureClient/JobStateUpdateClient.py | 23 +------------------ 3 files changed, 22 insertions(+), 37 deletions(-) delete mode 100644 docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClientNew.py diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClientNew.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClientNew.py deleted file mode 100644 index 6f07442dc68..00000000000 --- a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACClientNew.py +++ /dev/null @@ -1,15 +0,0 @@ -# DIRAC/WorkloadManagementSystem/Client/JobAnnotatorClient.py -from DIRAC.WorkloadManagementSystem.FutureClient.JobAnnotatorClient import ( - JobAnnotatorClient as futureJobAnnotatorClient, -) - -from DIRAC.Core.Base.Client import Client, createClient - - -@createClient("WorkloadManagement/JobAnnotator") -class JobAnnotatorClient(Client): - diracxClient = futureJobAnnotatorClient # <-- the only change - - def __init__(self, url=None, **kwargs): - super().__init__(**kwargs) - self.serverURL = url or "WorkloadManagement/JobAnnotator" diff --git a/src/DIRAC/Core/Security/DiracX.py b/src/DIRAC/Core/Security/DiracX.py index 7c5e099edc2..1a6339d7af5 100644 --- a/src/DIRAC/Core/Security/DiracX.py +++ b/src/DIRAC/Core/Security/DiracX.py @@ -6,6 +6,7 @@ "diracxTokenFromPEM", "executeRPCStub", "FutureClient", + "stripValueIfOK", ) import base64 @@ -141,6 +142,26 @@ def inner(self, *args, **kwargs): return inner +def stripValueIfOK(func): + """Decorator to remove S_OK["Value"] from the return value of a function if it is OK. + + This is done as some update functions return the number of modified rows in + the database. This likely not actually useful so it isn't supported in + DiracX. Stripping the "Value" key of the dictionary means that we should + get a fairly straight forward error if the assumption is incorrect. + """ + + @functools.wraps(func) + def wrapper(*args, **kwargs): + result = func(*args, **kwargs) + if result.get("OK"): + if result.get("Value") is None: + result.pop("Value") + return result + + return wrapper + + def executeRPCStub(stub: dict): className = stub.get("dCls") modName = stub.get("dMod") diff --git a/src/DIRAC/WorkloadManagementSystem/FutureClient/JobStateUpdateClient.py b/src/DIRAC/WorkloadManagementSystem/FutureClient/JobStateUpdateClient.py index b674ca39f17..5dd9b99ebea 100644 --- a/src/DIRAC/WorkloadManagementSystem/FutureClient/JobStateUpdateClient.py +++ b/src/DIRAC/WorkloadManagementSystem/FutureClient/JobStateUpdateClient.py @@ -1,11 +1,10 @@ from __future__ import annotations -import functools import time from datetime import datetime, timezone from typing import TYPE_CHECKING -from DIRAC.Core.Security.DiracX import DiracXClient, FutureClient, addRPCStub +from DIRAC.Core.Security.DiracX import DiracXClient, FutureClient, addRPCStub, stripValueIfOK from DIRAC.Core.Utilities.ReturnValues import convertToReturnValue, returnValueOrRaise from DIRAC.Core.Utilities.TimeUtilities import DiracTime, fromString from DIRAC.WorkloadManagementSystem.Client import JobStatus @@ -14,26 +13,6 @@ from diracx.client.models import JobCommand -def stripValueIfOK(func): - """Decorator to remove S_OK["Value"] from the return value of a function if it is OK. - - This is done as some update functions return the number of modified rows in - the database. This likely not actually useful so it isn't supported in - DiracX. Stripping the "Value" key of the dictionary means that we should - get a fairly straight forward error if the assumption is incorrect. - """ - - @functools.wraps(func) - def wrapper(*args, **kwargs): - result = func(*args, **kwargs) - if result.get("OK"): - if result.get("Value") is None: - result.pop("Value") - return result - - return wrapper - - class JobStateUpdateClient(FutureClient): @convertToReturnValue def sendHeartBeat(self, jobID: str | int, dynamicData: dict, staticData: dict): From 8827423c761b53e9a38281a5bae8f5f176b39c18 Mon Sep 17 00:00:00 2001 From: aldbr Date: Wed, 4 Nov 2026 18:04:32 +0100 Subject: [PATCH 3/3] docs: fix FutureClient guide toctree and align it with the code - Fix the toctree entry indentation that broke the docs build. - Use the real CS option /DiracX/FutureClientEnabled// (opt-in, yes = DiracX) in a new "Enable the FutureClient" step. - Router example: add the access policy check and map JobNotFoundError to 404. - Logic example: check all job IDs before writing, one insert. - Explain addRPCStub and stripValueIfOK, and the DiracX release chain (regenerate client, release, bump pin, server version). - Show the legacy Client change with emphasize-lines instead of a duplicated file; reflow prose to 120 columns. Assisted-by: Claude:claude-opus-5-5 --- .../DIRACFutureClient.py | 5 +- .../DevelopingFutureClients/DiracXLogic.py | 34 ++- .../DevelopingFutureClients/DiracXRouter.py | 25 +- .../DevelopingFutureClients/index.rst | 219 +++++++++++------- .../DIRACToDiracXBridge/index.rst | 6 +- 5 files changed, 185 insertions(+), 104 deletions(-) diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACFutureClient.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACFutureClient.py index fd37416a74c..a495f191dc3 100644 --- a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACFutureClient.py +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DIRACFutureClient.py @@ -1,17 +1,18 @@ # DIRAC/WorkloadManagementSystem/FutureClient/JobAnnotatorClient.py from __future__ import annotations -from DIRAC.Core.Security.DiracX import DiracXClient, FutureClient, addRPCStub +from DIRAC.Core.Security.DiracX import DiracXClient, FutureClient, addRPCStub, stripValueIfOK from DIRAC.Core.Utilities.ReturnValues import convertToReturnValue class JobAnnotatorClient(FutureClient): @addRPCStub + @stripValueIfOK @convertToReturnValue def addAnnotation(self, jobID: str | int, annotation: str): with DiracXClient() as api: # Legacy accepts str or int, DiracX expects an int: - api.jobs.add_annotation({int(jobID): annotation}) + api.jobs.add_annotations({int(jobID): annotation}) @convertToReturnValue def getAnnotations(self, jobID: str | int): diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXLogic.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXLogic.py index 2ad15089e06..0d41f77c994 100644 --- a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXLogic.py +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXLogic.py @@ -1,23 +1,35 @@ # diracx-logic/src/diracx/logic/jobs/annotations.py from __future__ import annotations -from diracx.core.exceptions import NotReadyError +from diracx.core.exceptions import JobNotFoundError +from diracx.core.models import VectorSearchOperator, VectorSearchSpec from diracx.db.sql import JobDB +async def _check_jobs_exist(job_ids: list[int], job_db: JobDB) -> None: + """Raise JobNotFoundError if any of the given jobs does not exist.""" + search_query: VectorSearchSpec = { + "parameter": "JobID", + "operator": VectorSearchOperator.IN, + "values": job_ids, + } + _, results = await job_db.search(parameters=["JobID"], search=[search_query], sorts=[]) + found_job_ids = {int(result["JobID"]) for result in results} + if missing := sorted(set(job_ids) - found_job_ids): + raise JobNotFoundError(missing[0]) + + async def add_annotations(annotations: dict[int, str], job_db: JobDB) -> None: """Attach an annotation to each of the given jobs.""" - # Extremely efficient bulk operation. O(-n) probably - for job_id, annotation in annotations.items(): - if not await job_db.exists(job_id): - raise NotReadyError("I wasn't prepared, you took me by surprise") - - await job_db.insert_annotation(job_id, annotation) + # Check all the jobs first, so that nothing is written if one is missing + await _check_jobs_exist(list(annotations), job_db) + # One query for all the jobs. An earlier draft was O(-n), probably. + # (made up JobDB method, see the note below) + await job_db.insert_annotations(annotations) async def get_annotations(job_id: int, job_db: JobDB) -> list[str]: """Return all the annotations of a job.""" - if not await job_db.exists(job_id): - raise NotReadyError("AHHHHH") - - return await job_db.fetch_annotations(job_id) + await _check_jobs_exist([job_id], job_db) + # Made up JobDB method, see the note below + return await job_db.get_annotations(job_id) diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXRouter.py b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXRouter.py index c2dc537f2df..53d916b0f01 100644 --- a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXRouter.py +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/DiracXRouter.py @@ -4,42 +4,47 @@ from http import HTTPStatus from typing import Annotated -from diracx.core.exceptions import NotReadyError +from diracx.core.exceptions import JobNotFoundError from diracx.db.sql import JobDB from diracx.logic.jobs import add_annotations as add_annotations_bl from diracx.logic.jobs import get_annotations as get_annotations_bl from fastapi import Body, HTTPException from ..fastapi_classes import DiracxRouter +from .access_policies import ActionType, CheckWMSPolicyCallable router = DiracxRouter() @router.post("/annotations", status_code=HTTPStatus.NO_CONTENT) -async def add_annotation( +async def add_annotations( annotations: Annotated[dict[int, str], Body()], job_db: JobDB, + check_permissions: CheckWMSPolicyCallable, ): - """Attach an annotation to one or more jobs. What's an annotation? Who knows""" + """Attach an annotation to one or more jobs. - # MAGIC LINES PROBABLY WRITTEN AND REVIWED BY CLAUDE 🦀 + The body is a mapping of job ID to annotation text. + """ + # Every route checks permissions before doing anything else + await check_permissions(action=ActionType.MANAGE, job_db=job_db, job_ids=list(annotations)) try: await add_annotations_bl(annotations, job_db) - except NotReadyError as e: - raise HTTPException(status_code=HTTPStatus.PAYMENT_REQUIRED, detail=str(e)) from e + except JobNotFoundError as e: + raise HTTPException(status_code=HTTPStatus.NOT_FOUND, detail=str(e)) from e @router.get("/{job_id}/annotations") async def get_annotations( job_id: int, job_db: JobDB, + check_permissions: CheckWMSPolicyCallable, ) -> list[str]: """Return the annotations of a job.""" - - # MORE MAGIC LINES. DID YOU EXPECT SOMETHING DIFFERENT? + await check_permissions(action=ActionType.READ, job_db=job_db, job_ids=[job_id]) try: return await get_annotations_bl(job_id, job_db) - except NotReadyError as e: - raise HTTPException(status_code=HTTPStatus.PAYMENT_REQUIRED, detail=str(e)) from e + except JobNotFoundError as e: + raise HTTPException(status_code=HTTPStatus.NOT_FOUND, detail=str(e)) from e diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/index.rst b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/index.rst index b76c8cda7b9..48ea352216f 100644 --- a/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/index.rst +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/DevelopingFutureClients/index.rst @@ -2,11 +2,10 @@ Developing Future Clients ====================================== -This is a step by step guide to develop proper DIRAC *FutureClients* for a -seemless DIRAC-DiracX compatibility. +This is a step by step guide to develop proper DIRAC *FutureClients* for a seamless DIRAC-DiracX compatibility. -A ``FutureClient`` is a replacement of a legacy DIRAC ``Client`` that is capable of -talking to DiracX in a transparent way for the user. +A ``FutureClient`` is a replacement of a legacy DIRAC ``Client`` that is capable of talking to DiracX in a transparent +way for the user. .. contents:: :local: @@ -18,10 +17,8 @@ Overview DIRAC and DiracX access points are organized differently: -* **DIRAC** exposes a service through a **Handler** (the implementation) and a - **Client** (the access point). -* **DiracX** has no handlers. It has an **API** that uses the ``logic`` through - the ``routers``. +* **DIRAC** exposes a service through a **Handler** (the implementation) and a **Client** (the access point). +* **DiracX** has no handlers. It has an **API** that uses the ``logic`` through the ``routers``. The call chain of a migrated service is: @@ -51,12 +48,11 @@ Before writing any code, find out exactly what the legacy service does. DIRAC exposes a service through two parts: -* The **Handler**, located in ``Service/Handler.py``, which contains the - real implementation. Each remotely callable method is called - ``export_`` and its accepted argument types are declared in - ``types_`` right above. -* The **Client**, located in ``Client/Client.py``, is almost always - empty and only has a decorator that points to the Handler. +* The **Handler**, located in ``Service/Handler.py``, which contains the real implementation. Each remotely + callable method is called ``export_`` and its accepted argument types are declared in ``types_`` right + above. +* The **Client**, located in ``Client/Client.py``, is almost always empty and only has a decorator that points to + the Handler. In this example, ``WorkloadManagement/JobAnnotator`` points to ``WorkloadManagementSystem/Service/JobAnnotatorHandler.py``. @@ -73,21 +69,18 @@ And the legacy Client: :language: python :linenos: -Write down, for every ``export_*`` method of the Handler, what parameters it -receives and what it returns. +Write down, for every ``export_*`` method of the Handler, what parameters it receives and what it returns. -While developing the ``FutureClient``, the code -might differ a lot, but the inputs and outputs **MUST** stay exactly the same as the -one in the legacy client. +While developing the ``FutureClient``, the code might differ a lot, but the inputs and outputs **MUST** stay exactly the +same as the one in the legacy client. The only accepted exception is the unused return value of methods that change +something, see ``@stripValueIfOK`` in Step 4. Step 2: Check current DiracX API ======================================= -Now check whether DiracX already provides everything you need. Look at the -routers of the DiracX API, in -``diracx-routers/src/diracx/routers//``. A group is built in its -``__init__.py`` by including several routers. For example, the ``jobs`` group -looks like this: +Now check whether DiracX already provides everything you need. Look at the routers of the DiracX API, in +``diracx-routers/src/diracx/routers//``. A group is built in its ``__init__.py`` by including several routers. +For example, the ``jobs`` group looks like this: .. code-block:: python @@ -99,8 +92,8 @@ looks like this: router.include_router(query_router) router.include_router(submission_router) -Every endpoint of those routers is available in the DiracX API as -``api..``. For example, a router function: +Every endpoint of those routers is available in the DiracX API as ``api..``. For example, a router +function: .. code-block:: python @@ -114,22 +107,18 @@ will be called from DIRAC as: with DiracXClient() as api: api.jobs.do_something(...) -Try to map each legacy method to a DiracX operation. The mapping is **NOT** always -one to one: +Try to map each legacy method to a DiracX operation. The mapping is **NOT** always one to one: * DiracX endpoints are modern and might mix or split DIRAC handler's methods. -* The arguments usually need to be reshaped. For example, there are DiracX endpoints - that take a dictionary keyed by ``jobID``, so many jobs can be updated at once, - something that DIRAC doesn't usually do. -* The returned value can be different and may need to be converted back to the - legacy format. +* The arguments usually need to be reshaped. For example, there are DiracX endpoints that take a dictionary keyed by + ``jobID``, so many jobs can be updated at once, something that DIRAC doesn't usually do. +* The returned value can be different and may need to be converted back to the legacy format. -Then decide if every legacy method can be mapped or can use a combination of APIs. If so, then **skip Step 3** -and go directly to Step 4. -If something is missing, continue with Step 3. +Then decide if every legacy method can be mapped or can use a combination of APIs. If so, then **skip Step 3** and go +directly to Step 4. If something is missing, continue with Step 3. -For the ``JobAnnotator`` example, since it's made up, there is nothing about it -in any of the existing routers, so we need Step 3. +For the ``JobAnnotator`` example, since it's made up, there is nothing about it in any of the existing routers, so we +need Step 3. Step 3: Develop new DiracX API ===================================== @@ -137,14 +126,13 @@ Step 3: Develop new DiracX API The API is split in two layers: * **logic** (``diracx-logic``): the business logic, independent of HTTP. -* **routers** (``diracx-routers``): the HTTP endpoints. They check - permissions, call the logic and translate exceptions into HTTP errors. +* **routers** (``diracx-routers``): the HTTP endpoints. They check permissions, call the logic and translate exceptions + into HTTP errors. 3.1 Where does the new code go? ------------------------------- -In this example the annotations belong to jobs, so the new code goes into the -existing ``jobs`` group of DiracX: +In this example the annotations belong to jobs, so the new code goes into the existing ``jobs`` group of DiracX: .. code-block:: text @@ -152,35 +140,60 @@ existing ``jobs`` group of DiracX: ├── diracx-logic/src/diracx/logic/jobs/annotations.py └── diracx-routers/src/diracx/routers/jobs/annotations.py -Keep both layers in the same group directory (``jobs``) and with the same file -name (``annotations.py``). +Keep both layers in the same group directory (``jobs``) and with the same file name (``annotations.py``). .. important:: - ``jobs`` is only the right place **for this example**. For other - developments, the new code might need to go to another existing directory - (such as pilots or auth), or even to a new group directory. + ``jobs`` is only the right place **for this example**. For other developments, the new code might need to go to + another existing directory (such as pilots or auth), or even to a new group directory. - Keep in mind that the group is also the namespace of the DiracX - client: code placed in ``routers//`` is called from DIRAC as - ``api..``, so choose the group name carefully, - as it becomes part of the public API. + Keep in mind that the group is also the namespace of the DiracX client: code placed in ``routers//`` is called + from DIRAC as ``api..``, so choose the group name carefully, as it becomes part of the public API. 3.2 Logic --------- +The logic layer is made of plain async functions. They receive the DB objects as arguments and know nothing about HTTP. + .. literalinclude:: DiracXLogic.py :language: python :linenos: +A few things to notice: + +* The endpoint takes many jobs at once, so the logic checks **all** of them before writing anything. A missing job must + not leave a partial write behind. +* It talks to the database with one query per call, not one per job. +* A missing job raises ``JobNotFoundError``, the same exception used by the existing ``jobs`` logic. + +.. note:: + + ``JobDB.insert_annotations`` and ``JobDB.get_annotations`` do not exist: they stand for whatever DB methods your + feature needs. New tables and DB methods go into ``diracx-db``, which is not covered by this guide. 3.3 Router ---------- +The router handles HTTP: it checks permissions, calls the logic and turns the logic exceptions into HTTP errors. + .. literalinclude:: DiracXRouter.py :language: python :linenos: +* **Permissions**: every route must call an access policy (here ``check_permissions`` with the ``ActionType`` matching + the operation) or be explicitly marked ``@open_access``. The DiracX test suite fails if a route does neither. Without + it, any authenticated user could read or change any job. +* **Errors**: ``JobNotFoundError`` becomes ``404 NOT FOUND``, as in the other ``jobs`` routes. On the DIRAC side, the + FutureClient turns it into an ``S_ERROR``. + +.. tip:: + + Once the shape of the endpoint is clear, the body of the logic and the router can be drafted with an AI coding + assistant. Which one is up to you: by the time you read this, there is probably a newer one. Whichever it is, it will + sound very sure of itself, and so will its magic lines. Review what it produces against the existing routers in the + same group (permission checks, error mapping, number of DB queries), as you would for any other contribution. The + reviewer is still a human, for now. + Then register the new router in the ``jobs`` group, next to the existing ones: .. code-block:: python @@ -197,29 +210,37 @@ Then register the new router in the ``jobs`` group, next to the existing ones: router.include_router(submission_router) router.include_router(annotations_router) # <-- new -Because ``annotations_router`` is included in the ``jobs`` router, its -endpoints end up in the ``jobs`` group of the DiracX API. +Because ``annotations_router`` is included in the ``jobs`` router, its endpoints end up in the ``jobs`` group of the +DiracX API. 3.4 DiracX API ----------------- -There is no code to write here. The DiracX API will need to be generated from the routers. - -Once your DiracX change is merged, the new endpoints appear in the +There is no code to write by hand here: the DiracX client is generated from the routers. The new endpoints appear in the client with the name of the router function, inside the group of the router: ====================== ====================================== ============================== Router function Endpoint DiracX API call ====================== ====================================== ============================== -``add_annotation`` ``POST /api/jobs/annotations`` ``api.jobs.add_annotation`` +``add_annotations`` ``POST /api/jobs/annotations`` ``api.jobs.add_annotations`` ``get_annotations`` ``GET /api/jobs/{job_id}/annotations`` ``api.jobs.get_annotations`` ====================== ====================================== ============================== +For DIRAC to be able to call them, several things have to happen, in this order: + +#. **Regenerate the client in the same DiracX pull request** as the router (see the DiracX developer documentation for + the ``generate-client`` command). The generated code is committed, so the PR is incomplete without it. +#. **Wait for a DiracX release** that contains the change. +#. **Bump the DiracX version required by DIRAC** to that release. Until then, ``api.jobs.add_annotations`` does not + exist in the client DIRAC installs, and calling it fails with an ``AttributeError``. +#. **Keep the server in mind**: the FutureClient only works against a DiracX server that is at least that version. An + older server answers ``404 NOT FOUND`` for the new endpoint, so the FutureClient must not be enabled (see `Step 6: + Enable the FutureClient`_) on an installation before its DiracX server has been upgraded. + Step 4: Code the new Future Client in DIRAC =========================================== -Create the FutureClient in the ``FutureClient`` directory of the same -system: +Create the FutureClient in the ``FutureClient`` directory of the same system: .. code-block:: text @@ -228,39 +249,81 @@ system: A FutureClient: * Inherits from ``FutureClient``. -* Implements **the same methods, with the same arguments** as the legacy - service. +* Implements **the same methods, with the same arguments** as the legacy service. * Calls DiracX through the ``DiracXClient`` context manager. .. literalinclude:: DIRACFutureClient.py :language: python :linenos: -Step 5: Change the DIRAC Client configuration -============================================= +* ``@convertToReturnValue`` turns the return value into ``S_OK(value)`` and any exception (including HTTP errors from + DiracX) into ``S_ERROR``, so callers get the usual DIRAC return structure. +* ``@addRPCStub`` goes on methods that **change** something (here ``addAnnotation``). It attaches an ``rpcStub`` to the + result, like a DISET call does. If the call fails, the caller can put this stub into a ``ForwardDISET`` request of the + Request Management System, which replays the call later. Methods that only read (here ``getAnnotations``) do not need + it. +* ``@stripValueIfOK`` also goes on methods that change something. Legacy handlers often return a value nobody uses for + writes (here the result of ``JobAnnotatorDB.addAnnotation``, often a number of modified rows), while the DiracX + endpoint returns nothing. Instead of returning ``S_OK(None)``, the decorator removes the ``Value`` key, so a caller + that does rely on the legacy value fails with a clear ``KeyError`` instead of silently getting ``None``. If a caller + really needs the value, return it from the FutureClient instead. -The legacy Client requires just a minimal change in its configuration -for it to see the new changes: +Step 5: Plug the FutureClient into the legacy Client +==================================================== -.. literalinclude:: DIRACClientNew.py - :language: python +The legacy Client needs one change: a ``diracxClient`` attribute that points to the FutureClient (highlighted lines): + +.. code-block:: python :linenos: + :emphasize-lines: 2-4,11 + + # DIRAC/WorkloadManagementSystem/Client/JobAnnotatorClient.py + from DIRAC.WorkloadManagementSystem.FutureClient.JobAnnotatorClient import ( + JobAnnotatorClient as futureJobAnnotatorClient, + ) + + from DIRAC.Core.Base.Client import Client, createClient -Now the FutureClient is ready to roll into production. Yay :) -Disabling the DiracX API ---------------------------- + @createClient("WorkloadManagement/JobAnnotator") + class JobAnnotatorClient(Client): + diracxClient = futureJobAnnotatorClient + + def __init__(self, url=None, **kwargs): + super().__init__(**kwargs) + self.serverURL = url or "WorkloadManagement/JobAnnotator" + +This change alone does **not** switch anything to DiracX: the legacy Client keeps calling the DIRAC Handler until the +FutureClient is enabled in the Configuration System. + +Step 6: Enable the FutureClient +=============================== + +Whether a Client uses the FutureClient or the legacy Handler is decided per service by the Configuration System option +``/DiracX/FutureClientEnabled//``: -The DiracX API can be disabled for a given service from the web interface -(the Configuration System), so DIRAC keeps using the legacy Client and Handler -for it. The option is located under ``DiracX/legacyClientEnabled``. +* ``yes`` (or ``true``, ``y``, ``1``): the Client calls DiracX through the FutureClient. +* ``no``, or option not set (the default): the Client calls the legacy DIRAC Handler. -For this example, to disable the use of the ``FutureClient``, the config would -be as follows: +For this example, to enable the ``FutureClient``: .. code-block:: text DiracX - └── legacyClientEnabled - └── WorkloadManagement - └── JobAnnotator = yes + { + FutureClientEnabled + { + WorkloadManagement + { + JobAnnotator = yes + } + } + } + +To go back to the legacy Handler, set the option to ``no`` or remove it. + +.. warning:: + + If the option is ``yes`` but the legacy Client has no ``diracxClient`` attribute (Step 5), every call made through + the Client raises ``NotImplementedError``. Enable the option only on installations running a DIRAC version that + contains Step 5, and a DiracX server that contains Step 3. diff --git a/docs/source/DeveloperGuide/DIRACToDiracXBridge/index.rst b/docs/source/DeveloperGuide/DIRACToDiracXBridge/index.rst index 28ba685a89d..76d873546e0 100644 --- a/docs/source/DeveloperGuide/DIRACToDiracXBridge/index.rst +++ b/docs/source/DeveloperGuide/DIRACToDiracXBridge/index.rst @@ -2,10 +2,10 @@ Bridging DIRAC and DiracX ========================= -This section explains how to bridge DIRAC and DiracX while the migration is -still on going and DiracX being actively developed. +This section explains how to bridge DIRAC and DiracX while the migration is still ongoing and DiracX being actively +developed. .. toctree:: :maxdepth: 1 - DevelopingFutureClients/index + DevelopingFutureClients/index