-
Notifications
You must be signed in to change notification settings - Fork 13
feat: update tiled node insertion structure #1669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||||||||||
| import json | ||||||||||||||||||||
| import logging | ||||||||||||||||||||
| from collections.abc import Mapping | ||||||||||||||||||||
| from dataclasses import dataclass | ||||||||||||||||||||
|
|
@@ -8,7 +9,9 @@ | |||||||||||||||||||
| from bluesky.callbacks.tiled_writer import TiledWriter | ||||||||||||||||||||
| from bluesky_stomp.messaging import StompClient | ||||||||||||||||||||
| from bluesky_stomp.models import Broker, DestinationBase, MessageTopic | ||||||||||||||||||||
| from fastapi import status | ||||||||||||||||||||
| from tiled.client import from_uri | ||||||||||||||||||||
| from tiled.client.utils import ClientError | ||||||||||||||||||||
|
|
||||||||||||||||||||
| from blueapi.cli.scratch import get_python_environment | ||||||||||||||||||||
| from blueapi.config import ApplicationConfig, OIDCConfig, ServiceAccount, StompConfig | ||||||||||||||||||||
|
|
@@ -25,6 +28,7 @@ | |||||||||||||||||||
| TaskRequest, | ||||||||||||||||||||
| WorkerTask, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| from blueapi.utils import TILED_PROPOSAL_RE | ||||||||||||||||||||
| from blueapi.utils.serialization import access_blob | ||||||||||||||||||||
| from blueapi.worker.event import ProgressEvent, TaskStatusEnum, WorkerEvent, WorkerState | ||||||||||||||||||||
| from blueapi.worker.task import Task | ||||||||||||||||||||
|
|
@@ -205,7 +209,43 @@ def begin_task( | |||||||||||||||||||
| api_key=tiled_config.authentication, | ||||||||||||||||||||
| headers=pass_through_headers, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if task.task_id is not None: | ||||||||||||||||||||
| task_ = get_task_by_id(task_id=task.task_id) | ||||||||||||||||||||
| if task_ is not None: | ||||||||||||||||||||
| task_metadata = task_.task.metadata | ||||||||||||||||||||
| instrument = active_context.run_engine.md["instrument"] | ||||||||||||||||||||
| instrument_session = task_metadata["instrument_session"] | ||||||||||||||||||||
| if not (match := TILED_PROPOSAL_RE.match(instrument_session)): | ||||||||||||||||||||
| raise ValueError("Invalid instrument session") | ||||||||||||||||||||
| proposal = match["proposal"] | ||||||||||||||||||||
| # Each level's access blob is the prefix of the full | ||||||||||||||||||||
| # (beamline, proposal, visit) one that access_blob() builds, | ||||||||||||||||||||
| # matching the beamline/proposal/session tiers the tiled | ||||||||||||||||||||
| # access policy expects a container to be tagged with. | ||||||||||||||||||||
| session_blob = json.loads(access_blob(instrument_session, instrument)) | ||||||||||||||||||||
| level_access_tags = [ | ||||||||||||||||||||
| [json.dumps({"beamline": instrument})], | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be a bit late at this stage but why are we using |
||||||||||||||||||||
| [json.dumps({"beamline": instrument, "proposal": proposal})], | ||||||||||||||||||||
| [json.dumps(session_blob)], | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think having It's also going from dict -> json -> dict -> json where it probably shouldn't |
||||||||||||||||||||
| ] | ||||||||||||||||||||
| for key, access_tags in zip( | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could |
||||||||||||||||||||
| (instrument, proposal, instrument_session), | ||||||||||||||||||||
| level_access_tags, | ||||||||||||||||||||
| strict=True, | ||||||||||||||||||||
| ): | ||||||||||||||||||||
| if key not in tiled_client: | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| tiled_client.create_container( | ||||||||||||||||||||
| key=key, access_tags=access_tags | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| except ClientError as e: | ||||||||||||||||||||
| if ( | ||||||||||||||||||||
| e.response.status_code == status.HTTP_409_CONFLICT | ||||||||||||||||||||
| ): # already exists | ||||||||||||||||||||
| ... | ||||||||||||||||||||
| else: | ||||||||||||||||||||
| raise | ||||||||||||||||||||
|
Comment on lines
+242
to
+247
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||
| tiled_client = tiled_client[key] | ||||||||||||||||||||
| tiled_writer_token = active_context.run_engine.subscribe( | ||||||||||||||||||||
| TiledWriter(tiled_client, batch_size=1) | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
@@ -230,7 +270,7 @@ def remove_callback_when_task_finished( | |||||||||||||||||||
| if task.task_id is not None: | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| active_worker.begin_task(task.task_id) | ||||||||||||||||||||
| except: | ||||||||||||||||||||
| except Exception: | ||||||||||||||||||||
| for channel, token in subscribers: | ||||||||||||||||||||
| channel.unsubscribe(token) | ||||||||||||||||||||
| raise | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,8 @@ | |
| Return = TypeVar("Return") | ||
|
|
||
| INSTRUMENT_SESSION_RE = re.compile(r"^[a-z]{2}(?P<proposal>\d+)-(?P<visit>\d+)$") | ||
| # Full proposal code (e.g. "cm12345" from "cm12345-1") for building tiled node paths. | ||
| TILED_PROPOSAL_RE = re.compile(r"^(?P<proposal>[a-z]{2}\d+)-\d+$") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we combine this with the other regex somehow instead of having two that do subtly different things? Something like |
||
|
|
||
|
|
||
| def report_successful_devices( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ services: | |
| bundles: | ||
| diamond-policies: | ||
| service: ghcr | ||
| resource: ghcr.io/diamondlightsource/authz-policy:0.0.24 | ||
| resource: ghcr.io/zohebshaikh/authz-policy:0.0.25-alpha | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR here |
||
| polling: | ||
| min_delay_seconds: 30 | ||
| max_delay_seconds: 120 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |
| TaskResponse, | ||
| WorkerTask, | ||
| ) | ||
| from blueapi.utils import TILED_PROPOSAL_RE | ||
| from blueapi.worker.event import ( | ||
| TaskResult, | ||
| TaskStatus, | ||
|
|
@@ -354,7 +355,7 @@ def test_task_metadata_propagated( | |
| "user": User.alice, | ||
| "instrument_session": VALID_INSTRUMENT_SESSION[User.alice], | ||
| "tiled_access_tags": [ | ||
| '{"proposal": 12345, "visit": 1, "beamline": "adsim"}', | ||
| '{"proposal": "cm12345", "visit": 1, "beamline": "adsim"}', | ||
| ], | ||
| "blueapi_task_id": response.task_id, | ||
| } | ||
|
|
@@ -612,7 +613,12 @@ def on_event(event: AnyEvent) -> None: | |
| assert stream_resource["run_start"] == start_doc["uid"] | ||
| assert stream_resource["uri"] == f"file://localhost/tmp/adsim-{scan_id}-det.h5" | ||
|
|
||
| tiled_url = f"http://localhost:8407/api/v1/metadata/{start_doc['uid']}" | ||
| proposal = TILED_PROPOSAL_RE.match(start_doc["instrument_session"])["proposal"] # type: ignore | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this type ignore still needed? |
||
| tiled_url = ( | ||
| "http://localhost:8407/api/v1/metadata/" | ||
| f"{start_doc['instrument']}/{proposal}/{start_doc['instrument_session']}/" | ||
| f"{start_doc['uid']}" | ||
| ) | ||
| response = requests.get( | ||
| tiled_url, headers={"authorization": "Bearer " + get_access_token(user)} | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we get this far and there is no task_id, nothing is going to work. It would be better to remove this check and the one on line 270 and replace them with
and then we wouldn't have to worry about it for the rest of the function.