feat: update tiled node insertion structure - #1669
ZohebShaikh wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## tiled-update #1669 +/- ##
================================================
+ Coverage 96.34% 96.39% +0.05%
================================================
Files 46 46
Lines 3665 3690 +25
================================================
+ Hits 3531 3557 +26
+ Misses 134 133 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4414034 to
abcd7ff
Compare
| diamond-policies: | ||
| service: ghcr | ||
| resource: ghcr.io/diamondlightsource/authz-policy:0.0.24 | ||
| resource: ghcr.io/zohebshaikh/authz-policy:0.0.25-alpha |
a0df79b to
a5635b9
Compare
a5635b9 to
5b5bb6f
Compare
tpoliaw
left a comment
tpoliaw
left a comment
There was a problem hiding this comment.
I don't really know how the authorization stuff for tiled works but it looks plausible.
The amount of stuff going on in the begin_task is starting to look a bit suspect though. Can be a later PR but is it possible offload some of it to the tiled writer or client?
| headers=pass_through_headers, | ||
| ) | ||
|
|
||
| if task.task_id is not None: |
There was a problem hiding this comment.
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
if not task.task_id:
raise ValueError("No task_id provided")and then we wouldn't have to worry about it for the rest of the function.
|
|
||
| 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+$") |
There was a problem hiding this comment.
Can we combine this with the other regex somehow instead of having two that do subtly different things?
Something like ^(?P<proposal>(?P<code>[a-z]{2})(?P<proposal_number>\d+))-(?P<visit>\d+)$ would give us code, proposal, proposal_number and visit so it could be used for both.
| level_access_tags = [ | ||
| [json.dumps({"beamline": instrument})], | ||
| [json.dumps({"beamline": instrument, "proposal": proposal})], | ||
| [json.dumps(session_blob)], |
There was a problem hiding this comment.
I don't think having access_blob as its own function makes sense any more. It should either be updated to return all three of these, or the proposal/visit/beamline line should be inlined here.
It's also going from dict -> json -> dict -> json where it probably shouldn't
| # 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})], |
There was a problem hiding this comment.
might be a bit late at this stage but why are we using beamline instead of instrument?
| if ( | ||
| e.response.status_code == status.HTTP_409_CONFLICT | ||
| ): # already exists | ||
| ... | ||
| else: | ||
| raise |
There was a problem hiding this comment.
| if ( | |
| e.response.status_code == status.HTTP_409_CONFLICT | |
| ): # already exists | |
| ... | |
| else: | |
| raise | |
| if response.status_code != status.HTTP_409_CONFLICT: | |
| raise | |
| # else: node already exists, continue |
| [json.dumps({"beamline": instrument, "proposal": proposal})], | ||
| [json.dumps(session_blob)], | ||
| ] | ||
| for key, access_tags in zip( |
There was a problem hiding this comment.
Could level_access_tags be a dict instead of having to immediately zip the list with something else?
| 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 |
There was a problem hiding this comment.
is this type ignore still needed?
2 participants
Stack created with GitHub Stacks CLI • Give Feedback 💬