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

Trouble understanding resolve? #170

Open
dlwh opened this issue Jun 20, 2024 · 9 comments
Open

Trouble understanding resolve? #170

dlwh opened this issue Jun 20, 2024 · 9 comments

Comments

@dlwh
Copy link

dlwh commented Jun 20, 2024

Hi, I feel like I don't really understand what resolve is supposed to do.

From the docs, it says it "Obtains updated bounds, subject to the cache policy." Now, I don't really see much documentation around the cache policy (probably user error?) but it'd be helpful to see an example of what is supposed to work.

Thanks!

import tempfile

import tensorstore as ts


def test_simple_resize_bounds():
    with tempfile.TemporaryDirectory() as tmpdir:
        store1 = ts.open(
                {
                    'driver': 'zarr',
                    'kvstore': {
                        'driver': 'file',
                        'path': tmpdir,
                    }
                },
                create=True,
                dtype=ts.int32,
                shape=[1000, 2000, 3000],
                chunk_layout=ts.ChunkLayout(inner_order=[2, 1, 0]),
        ).result()

        store2 = ts.open(
                {
                    'driver': 'zarr',
                    'kvstore': {
                        'driver': 'file',
                        'path': tmpdir,
                    }
                },
                dtype=ts.int32,
        ).result()

        assert store2.shape == (1000, 2000, 3000)
        assert store2.chunk_layout.inner_order == (2, 1, 0)

        store1 = store1.resize(exclusive_max=[2000, 3000, 4000]).result()

        assert store1.shape == (2000, 3000, 4000)

        store2 = store2.resolve(fix_resizable_bounds=True).result()

        assert store2.shape == (2000, 3000, 4000)  # nope?
@jbms
Copy link
Collaborator

jbms commented Jun 20, 2024

resolve is subject to the recheck_cached_metadata option specified when opening: https://google.github.io/tensorstore/driver/zarr/index.html#json-driver/zarr.recheck_cached_metadata

The default value is "open", which means the metadata won't be rechecked after opening (note that if you pass in a shared context object, the same cache can be shared across calls to open). In that case resolve won't attempt to read new metadata from storage, but it will pick up new metadata in the shared cache.

In order to have resolve actually read new metadata from storage, you would need to specify recheck_cached_metadata=True when opening. But that will make it also revalidate the metadata on every read/write operation, which is probably not what you want.

Your best option currently is probably: store2 = ts.open(store2.spec(retain_context=True), recheck_cached_metadata="open").result()

There is certainly room for improvement in the API here. Probably we need to introduce an API to change the recheck_cached_metadata value of an open TensorStore object ---- e.g. via an additional parameter to resolve. That has been on the TODO list for a while but we didn't prioritize it. If you need this functionality, we can try to prioritize it though.

Note: fix_resizable_bounds isn't relevant here --- that will mark the bounds as explicit rather than implicit but doesn't affect the bounds themselves.

@dlwh
Copy link
Author

dlwh commented Jun 20, 2024

Thanks for the fast response (and the great library!)

I can certainly work around via just reopening and that's probably good enough for my use case. I'm planning a janky-column store thing for data loading and being able to reread the bounds would be helpful, but I'm pretty sure it won't be the critical path pretty much ever.

@dlwh
Copy link
Author

dlwh commented Jun 20, 2024

(recheck_cached_metadata doesn't to seem to exist as an option for ts.open in 1.61. Guessing I need to go to head)

@jbms
Copy link
Collaborator

jbms commented Jun 20, 2024

(recheck_cached_metadata doesn't to seem to exist as an option for ts.open in 1.61. Guessing I need to go to head)

Looks like we forgot to expose that option on open. Instead you can do:

spec = store2.spec(retain_context=True)
spec.update(recheck_cached_metadata="open")
store2 = ts.open(spec)

In fact the update call is not needed except if you had set a different value of recheck_cached_metadata when opening in the first place.

@dlwh
Copy link
Author

dlwh commented Jun 20, 2024

That doesn't work either :(

>       spec.update(recheck_cached_metadata="open")
E       TypeError: update(): incompatible function arguments. The following argument types are supported:
E           1. (self: tensorstore.Spec, *, open_mode: Optional[tensorstore.OpenMode] = None, open: Optional[bool] = None, create: Optional[bool] = None, delete_existing: Optional[bool] = None, assume_metadata: Optional[bool] = None, assume_cached_metadata: Optional[bool] = None, unbind_context: Optional[bool] = None, strip_context: Optional[bool] = None, context: Optional[tensorstore.Context] = None, kvstore: Optional[tensorstore.KvStore.Spec] = None, rank: Optional[int] = None, dtype: Optional[tensorstore.dtype] = None, domain: Optional[tensorstore.IndexDomain] = None, shape: Optional[Sequence[int]] = None, chunk_layout: Optional[tensorstore.ChunkLayout] = None, codec: Optional[tensorstore.CodecSpec] = None, fill_value: Optional[numpy.typing.ArrayLike] = None, dimension_units: Optional[Sequence[Optional[Union[tensorstore.Unit, str, Real, Tuple[Real, str]]]]] = None, schema: Optional[tensorstore.Schema] = None) -> None
E       
E       Invoked with: Spec({
E         'cache_pool': ['cache_pool'],
E         'context': {
E           'cache_pool': {},
E           'data_copy_concurrency': {},
E           'file_io_concurrency': {},
E           'file_io_sync': True,
E         },
E         'data_copy_concurrency': ['data_copy_concurrency'],
E         'driver': 'zarr',
E         'dtype': 'int64',
E         'kvstore': {
E           'driver': 'file',
E           'file_io_concurrency': ['file_io_concurrency'],
E           'file_io_sync': ['file_io_sync'],
E           'path': '/var/folders/x_/l2t6wfrd3zq71npb1cvfp6v80000gn/T/tmpjhyaa7m8/a/offsets/',
E         },
E         'metadata': {
E           'chunks': [2048],
E           'compressor': {
E             'blocksize': 0,
E             'clevel': 5,
E             'cname': 'lz4',
E             'id': 'blosc',
E             'shuffle': -1,
E           },
E           'dimension_separator': '.',
E           'dtype': '<i8',
E           'fill_value': None,
E           'filters': None,
E           'order': 'C',
E           'shape': [1],
E           'zarr_format': 2,
E         },
E         'transform': {'input_exclusive_max': [[1]], 'input_inclusive_min': [0]},
E       }); kwargs: recheck_cached_metadata='open'

@dlwh
Copy link
Author

dlwh commented Jun 20, 2024

and if i omit it I get

E       ValueError: FAILED_PRECONDITION: Error opening "zarr" driver: Expected "shape" of [1] but received: [3] [source locations='tensorstore/driver/zarr/driver.cc:509\ntensorstore/driver/kvs_backed_chunk_driver.cc:1262\ntensorstore/driver/driver.cc:112'] [tensorstore_spec='{\"context\":{\"cache_pool\":{},\"data_copy_concurrency\":{},\"file_io_concurrency\":{},\"file_io_sync\":true},\"driver\":\"zarr\",\"dtype\":\"int64\",\"kvstore\":{\"driver\":\"file\",\"path\":\"/var/folders/x_/l2t6wfrd3zq71npb1cvfp6v80000gn/T/tmp8rw9hhkk/a/offsets/\"},\"metadata\":{\"chunks\":[2048],\"compressor\":{\"blocksize\":0,\"clevel\":5,\"cname\":\"lz4\",\"id\":\"blosc\",\"shuffle\":-1},\"dimension_separator\":\".\",\"dtype\":\"<i8\",\"fill_value\":null,\"filters\":null,\"order\":\"C\",\"shape\":[1],\"zarr_format\":2},\"transform\":{\"input_exclusive_max\":[[1]],\"input_inclusive_min\":[0]}}']

@dlwh
Copy link
Author

dlwh commented Jun 20, 2024

(Sorry in the actual test I gave you it's

E           ValueError: FAILED_PRECONDITION: Error opening "zarr" driver: Expected "shape" of [1000,2000,3000] but received: [2000,3000,4000] [source locations='tensorstore/driver/zarr/driver.cc:509\ntensorstore/driver/kvs_backed_chunk_driver.cc:1262\ntensorstore/driver/driver.cc:112'] [tensorstore_spec='{\"context\":{\"cache_pool\":{},\"data_copy_concurrency\":{},\"file_io_concurrency\":{},\"file_io_sync\":true},\"driver\":\"zarr\",\"dtype\":\"int32\",\"kvstore\":{\"driver\":\"file\",\"path\":\"/var/folders/x_/l2t6wfrd3zq71npb1cvfp6v80000gn/T/tmphs0vqo9n/\"},\"metadata\":{\"chunks\":[101,101,101],\"compressor\":{\"blocksize\":0,\"clevel\":5,\"cname\":\"lz4\",\"id\":\"blosc\",\"shuffle\":-1},\"dimension_separator\":\".\",\"dtype\":\"<i4\",\"fill_value\":null,\"filters\":null,\"order\":\"F\",\"shape\":[1000,2000,3000],\"zarr_format\":2},\"transform\":{\"input_exclusive_max\":[[1000],[2000],[3000]],\"input_inclusive_min\":[0,0,0]}}']

@jbms
Copy link
Collaborator

jbms commented Jun 20, 2024

Alright, well there are some fixes to the Python bindings needed...

Sorry for the repeated incorrect advice.

You can fix the shape mismatch error by doing: store2.spec(retain_context=True, minimal_spec=True)

@dlwh
Copy link
Author

dlwh commented Jun 20, 2024

I have definitely never given anyone incorrect advice in a GH issue before!

minimal_spec=True does indeed fix it!

Thanks!

copybara-service bot pushed a commit that referenced this issue Jun 21, 2024
#170

PiperOrigin-RevId: 645464725
Change-Id: Ie4fb43baa6a77ad91ac380b71bf5dcf4a813e6fe
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

No branches or pull requests

2 participants