Skip to content
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

Review StreamResource and StreamDatum Schemas #301

Merged
merged 17 commits into from
Apr 2, 2024

Conversation

genematx
Copy link
Contributor

@genematx genematx commented Mar 29, 2024

Review the schemas of Stream Resource and Stream Datum documents to facilitate storage and client access.

Description

  • Remove path_semantics from Stream Resource and Stream Datum schemas.
  • Replace root and resource_path with uri. It may be a path on the local filesystem, file://localhost/{path}, a path on a shared filesystem file://{host}/{path}, to be remapped at read time via local mount config, or a non-file-based resource like s3://....
  • Rename spec to mimetype and use MIME type values like application/x-hdf5. It is expected that any information about options like SWMR will be passed in the resource_kwargs (parameters, see below).
  • Rename resource_kwargs to parameters. The word resource_ is a bit redundant here, and the word kwargs is a Python-ism.

Motivation and Context

This PR is based on the discussion and agreed recommendations made in Issue #296. The main motivation is to simplify and future-proof the access to data by consumers and clients, such as Tiled.

How Has This Been Tested?

Using existing tests (with renamed/replaced variables).

@genematx genematx marked this pull request as ready for review April 2, 2024 15:08
Copy link
Contributor

@DiamondJoseph DiamondJoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in #296 I'm happy with the changes to go in, and as discussed with @coretl just now, I'm happy for this to go in so long as ophyd-async is pinned to <2.0 until after our experiment in June, then I'll adjust our deployed services. The implementation looks correct, just one repeated nit to make the history of changes a little clearer.

event_model/tests/test_em.py Outdated Show resolved Hide resolved
event_model/tests/test_em.py Outdated Show resolved Hide resolved
event_model/tests/test_run_router.py Outdated Show resolved Hide resolved
genematx and others added 3 commits April 2, 2024 11:57
Co-authored-by: DiamondJoseph <53935796+DiamondJoseph@users.noreply.github.com>
Co-authored-by: DiamondJoseph <53935796+DiamondJoseph@users.noreply.github.com>
Co-authored-by: DiamondJoseph <53935796+DiamondJoseph@users.noreply.github.com>
Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some minor edits. Then I think this is good to go in.

docs/user/explanations/data-model.rst Outdated Show resolved Hide resolved
event_model/tests/test_em.py Outdated Show resolved Hide resolved
event_model/tests/test_em.py Outdated Show resolved Hide resolved
event_model/tests/test_run_router.py Outdated Show resolved Hide resolved
genematx and others added 4 commits April 2, 2024 14:41
Co-authored-by: Dan Allan <daniel.b.allan@gmail.com>
Co-authored-by: Dan Allan <daniel.b.allan@gmail.com>
Co-authored-by: Dan Allan <daniel.b.allan@gmail.com>
@genematx genematx merged commit af8fac3 into bluesky:main Apr 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants