Capabilities interface #113

aliddell · 2023-11-17T21:46:19Z

Expose and test Capabilities interface in Python.
Remove a bunch of commented code in camera.rs.

Note: Storage metadata is unqueryable unless you start the runtime (issue here). In the test test_storage_capabilities, it was necessary to start the runtime for the test to pass, but that's really pretty terrible and I wouldn't choose to do that.

… mentioned.

…tests.

…ow changes.

…sing).

…ities

…stent offset.{x,y}.high

aliddell · 2023-11-21T22:18:20Z

See checks for 79f117c marked "pull_request", rather than "pull_request_target".

andy-sweet

Zarr V3 stuff looks good other than some nits/optional suggestions.

Still need to take a closer look at the capabilities stuff. From a quick look at the implementation and tests, I think it looks fine, though maybe the implementation is more verbose than I was expecting.

Optional, but I do think this would be easier to review if it was split into 2 (or maybe 3) PRs. One for Zarr V3 support + tests, one for capabilities, and maybe an extra one for some of the other stuff (version, readme updates, clean up).

tests/test_basic.py

…ities

aliddell · 2023-11-22T14:56:54Z

Optional, but I do think this would be easier to review if it was split into 2 (or maybe 3) PRs. One for Zarr V3 support + tests, one for capabilities, and maybe an extra one for some of the other stuff (version, readme updates, clean up).

FWIW, I opened it when I thought it would be smaller and get in faster. Happy to split.

Co-authored-by: Andy Sweet <[email protected]>

Revert minor version bump.

This reverts commit b3c90dd.

…ities

andy-sweet

After a first pass, this mostly looks fine. I focused on the Python class definitions, rather than the binding implementations. The only one that looked like slightly strange/unexpected is TriggerCapabilities (see the line comment for more details).

src/camera.rs

src/capabilities.rs

andy-sweet · 2023-11-27T17:05:12Z

src/camera.rs

+#[pyclass]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct CameraCapabilities {
+    #[pyo3(get, set)]


Should most (maybe all?) of the Python attributes/properties in this PR be read-only (i.e. pyo3(get))?

Or do we need the set internally?

We don't actually need set internally. Will fix.

andy-sweet

Looks good to me now. I have a nit to pick around the ChunkingShardingCapabilities class name, but I'll leave that up to you.

Given the amount of new public API here, I think it could be worth @nclack taking a pass over this too.

andy-sweet · 2023-11-27T19:37:15Z

python/acquire/acquire.pyi

-    height: int
-    planes: int
+class StorageCapabilities:
+    chunk_dims_px: ChunkingShardingCapabilities


Nit: I don't love the name of the type here because it's long and includes two needs (that are the same right now, but might diverge in the future?). Is it worth using the same underlying private definition, but defining separately named classes for ChunkingCapabilities and ShardingCapabilities?

Similar comment for ChunkingShardingDims.

Agreed. I like the idea of exposing ChunkingCapabilities and ShardingCapabilities even though they derive from the same private type.

nclack

ChunkingShardingCapabilities should be changed but otherwise looks good. I'm having a little trouble running tests locally but it's probably something I'm doing wrong.

nclack · 2023-11-27T21:51:50Z

.gitignore

@@ -4,6 +4,7 @@ imgui.ini
 Testing
 *.zarr
 *.tif
+*.bin


where's the bin coming from?

Writing out a raw file in one of the tests.

nclack · 2023-11-28T16:39:55Z

python/acquire/acquire.pyi

-    height: int
-    planes: int
+class StorageCapabilities:
+    chunk_dims_px: ChunkingShardingCapabilities


Agreed. I like the idea of exposing ChunkingCapabilities and ShardingCapabilities even though they derive from the same private type.

…dingCapabilities. Split ChunkingShardingDims into ChunkDims and ShardDims.

aliddell added 11 commits November 17, 2023 11:41

Update chunking API.

394f482

Fix up README so Python code works as written and Spinnaker driver is…

0f40e26

… mentioned.

Add Zarr V3 tests. Add a note about environment variables in Zarr V3 …

7ccb9c3

…tests.

Bump minor release version.

459aea8

Update Zarr driver to v0.1.5.

8dbfa74

Recall that target: pull_request is what you want when testing workfl…

8fddb9f

…ow changes.

Update Zarr syntax in eGrabber tests.

e43b8eb

Remove a bunch of commented code.

62ba589

Expose AcquirePropertyMetadata as Capabilities (mypy and stubtest pas…

11247d7

…sing).

Tests for simulated camera capabilities.

317d52b

Merge remote-tracking branch 'upstream/main' into zarrv3-plus-capabil…

874bf9b

…ities

aliddell mentioned this pull request Nov 21, 2023

acquire_get_configuration_metadata fails to get Storage metadata unless the runtime has started acquire-project/acquire-video-runtime#119

Closed

aliddell added 4 commits November 21, 2023 16:11

Implement, but skip, testing storage metadata.

fefd5fc

Fix an image shape for simulated camera configuration to ensure consi…

6452f8b

…stent offset.{x,y}.high

Restore test_pr to pull_request_target.

dd26856

Unskip test_storage_capabilities.

79f117c

aliddell requested review from andy-sweet and nclack November 21, 2023 21:53

This was linked to issues Nov 21, 2023

expose capability api #33

Closed

update storage props for chunking/sharding api changes #106

Closed

Update acquire-python to reflect sharding storage transformer #53

Closed

aliddell marked this pull request as ready for review November 21, 2023 21:57

Restore test_pr to pull_request_target (again).

67b0205

andy-sweet reviewed Nov 22, 2023

View reviewed changes

tests/test_basic.py Outdated Show resolved Hide resolved

tests/test_basic.py Outdated Show resolved Hide resolved

tests/test_basic.py Outdated Show resolved Hide resolved

tests/test_basic.py Outdated Show resolved Hide resolved

aliddell added 2 commits November 22, 2023 09:54

Test formatting.

8cd2e2a

Merge remote-tracking branch 'upstream/main' into zarrv3-plus-capabil…

e096742

…ities

aliddell and others added 2 commits November 22, 2023 10:00

Merge remote-tracking branch 'upstream/main' into zarrv3

637e80f

Update tests/test_basic.py

8d919d8

Co-authored-by: Andy Sweet <[email protected]>

aliddell marked this pull request as draft November 22, 2023 15:50

aliddell added 5 commits November 22, 2023 10:51

Update test_pr.yml

72ae5e9

Update Cargo.toml

b3c90dd

Revert minor version bump.

Revert "Update Cargo.toml"

9b88453

This reverts commit b3c90dd.

Merge branch 'zarrv3' into zarrv3-plus-capabilities

22e1119

Merge remote-tracking branch 'upstream/main' into zarrv3-plus-capabil…

b0265bb

…ities

aliddell changed the title ~~Zarr v3 plus Capabilities interface~~ Capabilities interface Nov 22, 2023

aliddell marked this pull request as ready for review November 22, 2023 22:49

aliddell requested a review from andy-sweet November 22, 2023 22:50

andy-sweet reviewed Nov 22, 2023

View reviewed changes

src/camera.rs Outdated Show resolved Hide resolved

src/capabilities.rs Outdated Show resolved Hide resolved

aliddell added 2 commits November 27, 2023 11:14

Address PR comments.

63ebfae

Ignore files created by test_storage_capabilities.

0920d5d

andy-sweet reviewed Nov 27, 2023

View reviewed changes

Make Capabilities properties read-only.

1c102ee

aliddell force-pushed the zarrv3-plus-capabilities branch from b32fde5 to 1c102ee Compare November 27, 2023 18:39

aliddell requested a review from andy-sweet November 27, 2023 18:39

andy-sweet approved these changes Nov 27, 2023

View reviewed changes

nclack reviewed Nov 28, 2023

View reviewed changes

Split ChunkingShardingCapabilities into ChunkingCapabilities and Shar…

8fe4c0a

…dingCapabilities. Split ChunkingShardingDims into ChunkDims and ShardDims.

aliddell merged commit d71f3d0 into acquire-project:main Nov 28, 2023
20 checks passed

aliddell deleted the zarrv3-plus-capabilities branch November 28, 2023 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capabilities interface #113

Capabilities interface #113

aliddell commented Nov 17, 2023 •

edited

Loading

aliddell commented Nov 21, 2023

andy-sweet left a comment

aliddell commented Nov 22, 2023

andy-sweet left a comment

andy-sweet Nov 27, 2023

aliddell Nov 27, 2023

andy-sweet left a comment

andy-sweet Nov 27, 2023 •

edited

Loading

nclack Nov 28, 2023

nclack left a comment

nclack Nov 27, 2023

aliddell Nov 28, 2023

nclack Nov 28, 2023

@@ @@ -4,6 +4,7 @@ imgui.ini @@
               Testing
               *.zarr
               *.tif
+              *.bin

Capabilities interface #113

Capabilities interface #113

Conversation

aliddell commented Nov 17, 2023 • edited Loading

aliddell commented Nov 21, 2023

andy-sweet left a comment

Choose a reason for hiding this comment

aliddell commented Nov 22, 2023

andy-sweet left a comment

Choose a reason for hiding this comment

andy-sweet Nov 27, 2023

Choose a reason for hiding this comment

aliddell Nov 27, 2023

Choose a reason for hiding this comment

andy-sweet left a comment

Choose a reason for hiding this comment

andy-sweet Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

nclack Nov 28, 2023

Choose a reason for hiding this comment

nclack left a comment

Choose a reason for hiding this comment

nclack Nov 27, 2023

Choose a reason for hiding this comment

aliddell Nov 28, 2023

Choose a reason for hiding this comment

nclack Nov 28, 2023

Choose a reason for hiding this comment

aliddell commented Nov 17, 2023 •

edited

Loading

andy-sweet Nov 27, 2023 •

edited

Loading