Move creation of KV store dictionary to python layer #93
Conversation
.github/workflows/test_setup.yml
Outdated
| @@ -5,25 +5,25 @@ on: | |||
| branches: | |||
| - main | |||
| - branch-bug | |||
| - update-test-branch | |||
| - test-branch7 | |||
There was a problem hiding this comment.
What is branch 7? Is that a placeholder name, or a test for some specific number branch.
There was a problem hiding this comment.
@krisfed if you are not using any of these branches (branch-bug and test-branch7), please remove them. I think Abhi was using one of them but I don't see any pending changes.
| @@ -198,14 +198,12 @@ function makeZarrGroups(existingParentPath, newGroupsPath) | |||
| % Extract the S3 bucket name and path | |||
| [bucketName, objectPath] = obj.extractS3BucketNameAndPath(obj.Path); | |||
| % Create a Python dictionary for the KV store driver | |||
| RemoteStoreSchema = dictionary(["driver", "bucket", "path"], ["s3", bucketName, objectPath]); | |||
.github/workflows/test_setup.yml
Outdated
| @@ -5,25 +5,25 @@ on: | |||
| branches: | |||
| - main | |||
| - branch-bug | |||
| - update-test-branch | |||
| - test-branch7 | |||
There was a problem hiding this comment.
@krisfed if you are not using any of these branches (branch-bug and test-branch7), please remove them. I think Abhi was using one of them but I don't see any pending changes.
|
|
||
| jobs: | ||
| test: | ||
| runs-on: ${{matrix.os}} | ||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-14] | ||
| matlab-version: ['R2023b','R2024a'] |
There was a problem hiding this comment.
If 23b is working fine after your fix, please make sure to update this section: https://github.com/mathworks/MATLAB-support-for-Zarr-files?tab=readme-ov-file#mathworks-products-httpswwwmathworkscom
There was a problem hiding this comment.
Good point! Will check once we are able to run the tests on multiple releases, and update then (if no issues surface in the tests)
| - isRemote (bool): whether the resource to be accessed with this | ||
| KV store is remote (s3) or local | ||
| - objPath (str): path to local Zarr file or to s3 object | ||
| - bucketName (str): If file is remote, this should be the S3 bucket |
There was a problem hiding this comment.
consistency for using "S3" or "s3"?
There was a problem hiding this comment.
Good point - updating to "S3" for consistency
|
|
||
| jobs: | ||
| test: | ||
| runs-on: ${{matrix.os}} | ||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-14] | ||
| matlab-version: ['R2023b','R2024a'] | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v2 |
There was a problem hiding this comment.
Could you please update this to "v4" instead of "v2"?
https://github.com/actions/checkout?tab=readme-ov-file#checkout-v4
|
The test failure is due to the ongoing outage, so this can be ignored for now: |
| - update-test-branch | ||
| - test-branch7 | ||
| - test-branch8 | ||
| #on: |
There was a problem hiding this comment.
Approving as a temporary change due to the ongoing outage, but the tests were qualified locally.
Instead of creating a MATLAB dictionary and then converting it to Python dictionary, use a new function in ZarrPy to create a KV store in Python directly.
Fixes #67