Implement open_data and support tiff, netcdf and zarr for s3 and https protocol; Implement stack-mode which stacks items containing tiffs; #19

konstntokas · 2024-07-31T07:19:30Z

Closes #3, #8

1. Implement open_data and support tiff, netcdf and zarr for s3 and https protocol

I suggest to start with the notebooks in the folder examples as an introduction.

Here, I encountered a problem with the levels file in the server demo in xcube. When I try to open the levels file, I get a mldataset instance back. Wen I then try to open one level, an error occurs in xcube_server_stac_s3.ipynb cell 12. I suspect that when starting the server, the .levels file with Identifier local is used to calculate local_1w in resample_in_time.py, which adds <dim>_bnds data variables to the xr.Dataset, which in turn brings problems in levels. Any idea to mitigate that problem without changing the xcube server demo example?

Further I changes the github action workflow so that the the current xcube repo is checked out. Furthermore, I run the xcube server to get a higher test coverage. The unit tests work with decorators and are only run if the server is running. So for the later development, the server can be easily turned of, if needed.

Filtering the asset is not implemented yet. It is documented in Issue #18. I see this as a nice add-on for later. Also the stac-spec of an asset requires only href.

2. Implement stack-mode which stacks items containing tiffs

The data store parameter <stack_mode> is added. If True, the collection ID becomes the <data_id>. Items will selected with matches a bounding box and time range. These items will be stacked using odc-stac. So far only tiffs are supported. Reading of data in odc-stac relies on rasterio, which is based on GDAL. It should support formats, if a GDAL driver exists. Examples of other formats will be added in the future.

start pr draft for documentation

work in progress notebooks work again backup unittest adjusted error in dependencies test re-recorded cassettes for test_open_data_abfs test adjusted xcube server STAC example to open geotiff and levels mldataset now supported data_type implemented simplified data opener

forman

Very good! I have actually no serious complaints. Almost all my comments and suggestions regard style & design, only the one regarding the store ctor is mandatory.

You should add a coverage badge.

forman · 2024-08-05T13:45:49Z

environment_workflow.yml

+dependencies:
+  # Python
+  - python >=3.9
+  # Required xcube


Suggested change

# Required xcube

# Required by xcube

forman · 2024-08-05T13:46:03Z

environment_workflow.yml

+  - urllib3 >=1.26
+  - xarray >=2022.6
+  - zarr >=2.11
+# Required xcube-stac


Suggested change

# Required xcube-stac

# Required by xcube-stac

forman · 2024-08-05T13:54:53Z

xcube_stac/utils.py

+from .constants import DATA_OPENER_IDS, FloatInt, MAP_MIME_TYP_FORMAT
+
+
+def _get_assets_from_item(


I understand you are using underscore prefixes to indicate that the functions in this module are implementation details and not API.

However, underscored variables, functions, and classes should not be imported. Even by own modules (exceptions exists, e.g. testing). You can underscore, by renaming utils.py into _utils.py.

forman · 2024-08-05T13:59:42Z

xcube_stac/mldataset.py

+            self._datasets.append(_apply_scaling_nodata(ds, self._items))
+
+    def _get_num_levels_lazily(self) -> int:
+        return len(self._datasets)


Suggested change

return len(self._datasets)

return len(self._resolutions)

forman · 2024-08-05T14:01:15Z

xcube_stac/mldataset.py

+        return len(self._datasets)
+
+    def _get_dataset_lazily(self, index: int, parameters) -> xr.Dataset:
+        return self._datasets[index]


Suggested change

return self._datasets[index]

ds = odc.stac.load(

self._items,

resolution=self._resolutions[index],

**self._open_params,

)

return _apply_scaling_nodata(ds, self._items)

forman · 2024-08-05T14:18:14Z

xcube_stac/impl.py

+}
+
+
+class SingleImplementation:


Single Implementation of what? I recommend finding a better name of what is implemented here, for example, the Store Mode. Then StoreMode would describe the purely abstract interface, and SingleStoreMode represents its implementation for the mode Single.

Suggested change

class SingleImplementation:

class SingleStoreMode:

Changed the name to SingleStoreMode. To be consistent I also changed to name from StackImplementation to StackStoreMode.

forman · 2024-08-05T14:21:07Z

xcube_stac/impl.py

+                    f"Only 's3' and 'https' protocols are supported, not {protocol!r}. "
+                    f"The asset {asset.extra_fields['id']!r} has a href {asset.href!r}."


Suggested change

f"Only 's3' and 'https' protocols are supported, not {protocol!r}. "

f"The asset {asset.extra_fields['id']!r} has a href {asset.href!r}."

f"Only 's3' and 'https' protocols are supported, not {protocol!r}."

f" The asset {asset.extra_fields['id']!r} has a href {asset.href!r}."

forman · 2024-08-05T14:30:36Z

xcube_stac/impl.py

+        else:
+            if not self._s3_accessor.root == root:
+                LOG.debug(
+                    f"The bucket {self._s3_accessor.root!r} of the "
+                    f"S3 object storage changed to {root!r}. "
+                    "A new s3 data opener will be initialized."
+                )
+                self._s3_accessor = S3DataAccessor(
+                    root, storage_options=storage_options
+                )


Suggested change

else:

if not self._s3_accessor.root == root:

LOG.debug(

f"The bucket {self._s3_accessor.root!r} of the "

f"S3 object storage changed to {root!r}. "

"A new s3 data opener will be initialized."

)

self._s3_accessor = S3DataAccessor(

root, storage_options=storage_options

)

elif self._s3_accessor.root != root:

LOG.debug(

f"The bucket {self._s3_accessor.root!r} of the "

f"S3 object storage changed to {root!r}. "

"A new s3 data opener will be initialized."

)

self._s3_accessor = S3DataAccessor(

root, storage_options=storage_options

)

forman · 2024-08-05T14:31:20Z

xcube_stac/impl.py

+        else:
+            if not self._https_accessor.root == root:
+                LOG.debug(
+                    f"The root {self._https_accessor.root!r} of the "
+                    f"https data opener changed to {root!r}. "
+                    "A new https data opener will be initialized."
+                )
+                self._https_accessor = HttpsDataAccessor(root)


Suggested change

else:

if not self._https_accessor.root == root:

LOG.debug(

f"The root {self._https_accessor.root!r} of the "

f"https data opener changed to {root!r}. "

"A new https data opener will be initialized."

)

self._https_accessor = HttpsDataAccessor(root)

elif self._https_accessor.root != root:

LOG.debug(

f"The root {self._https_accessor.root!r} of the "

f"https data opener changed to {root!r}. "

"A new https data opener will be initialized."

)

self._https_accessor = HttpsDataAccessor(root)

forman · 2024-08-05T14:41:07Z

xcube_stac/store.py

+        elif stack_mode is True or stack_mode == "odc-stac":
+            self._impl = StackImplementation(
+                self._catalog, self._url_mod, self._searchable, self._storage_options_s3
+            )


Else? Since it is possible to instantiate the store via ctor, you should raise a ValueError() in the else-case. Otherwise _impl will not be initialised.

The data store parameters are validated against the schema in the xcube data store framework. the schema is define in constants.py and this is tested in (test_store.py)[https://github.com/xcube-dev/xcube-stac/blob/konstntokas-xxx-implement_stac_mode/test/test_store.py#L90].

However, I agree, if a user would initialize a stac data store directly by calling the class StacDataStore, then this is not covered. I therefore added a else case.

forman · 2024-08-05T14:59:28Z

I suspect that when starting the server, the .levels file with Identifier local is used to calculate local_1w in resample_in_time.py, which adds <dim>_bnds data variables to the xr.Dataset, which in turn brings problems in levels. Any idea to mitigate that problem without changing the xcube server demo example?

Let's address this separately.

konstntokas · 2024-08-06T08:03:33Z

I suspect that when starting the server, the .levels file with Identifier local is used to calculate local_1w in resample_in_time.py, which adds <dim>_bnds data variables to the xr.Dataset, which in turn brings problems in levels. Any idea to mitigate that problem without changing the xcube server demo example?

Let's address this separately.

I understood the problem better, and it is on the xcube-stac side. I agree addressing it seperately. It is documented in #20.

codecov · 2024-08-06T11:26:13Z

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

konstntokas · 2024-08-06T11:30:44Z

You should add a coverage badge.

Coverage badge is added. See https://app.codecov.io/gh/xcube-dev/xcube-stac/tree/konstntokas-xxx-implement_stac_mode. I need to double check the links once it is merged.

konstntokas added 17 commits July 10, 2024 11:02

tiff works, netcdf not

d8f2685

start pr draft for documentation

ready for rebase

1bbb317

backup

14fc51b

new workflow

5002dcf

new workflow 2

35c14f1

need for checkout

9609b95

open_params available for all data types and formats

942f8c8

workflow fix0

8ece8d0

run xcube server in CI

019f31a

run server in background

d78f06f

all tests are fine

21f1a88

ready for review

dd2e412

added cassette for unittest

b11304f

stacking mode implemented

16ec6f4

odc added to the requirement

82fd9e5

unittesting done for stack-mode

1eaecd0

konstntokas mentioned this pull request Jul 31, 2024

Implement open_data and support tiff, netcdf and zarr for s3 and https protocol #17

Closed

konstntokas added 2 commits July 31, 2024 17:47

all notebooks are running

0a09f8e

README updated

6740bd1

konstntokas requested a review from forman August 1, 2024 08:26

konstntokas marked this pull request as ready for review August 1, 2024 08:26

konstntokas added 2 commits August 1, 2024 16:56

meeting with Norman and Pont recap part 1

e19624b

mldataset precalculated with finest resolution found in the bands

3c57f02

forman requested changes Aug 5, 2024

View reviewed changes

konstntokas added 2 commits August 6, 2024 11:49

Norman's review addressed

5486a5b

badges added

fb467c4

konstntokas requested a review from forman August 6, 2024 11:32

cov badge link updated

3ba749b

konstntokas merged commit a54b3de into main Aug 19, 2024
3 checks passed

konstntokas deleted the konstntokas-xxx-implement_stac_mode branch August 19, 2024 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement open_data and support tiff, netcdf and zarr for s3 and https protocol; Implement stack-mode which stacks items containing tiffs; #19

Implement open_data and support tiff, netcdf and zarr for s3 and https protocol; Implement stack-mode which stacks items containing tiffs; #19

konstntokas commented Jul 31, 2024 •

edited

Loading

forman left a comment

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman Aug 5, 2024

konstntokas Aug 6, 2024

forman commented Aug 5, 2024

konstntokas commented Aug 6, 2024

codecov bot commented Aug 6, 2024

konstntokas commented Aug 6, 2024

		from .constants import DATA_OPENER_IDS, FloatInt, MAP_MIME_TYP_FORMAT


		def _get_assets_from_item(

-        return self._datasets[index]
+        ds = odc.stac.load(
+            self._items,
+            resolution=self._resolutions[index],
+            **self._open_params,
+        )
+        return _apply_scaling_nodata(ds, self._items)

		f"Only 's3' and 'https' protocols are supported, not {protocol!r}. "
		f"The asset {asset.extra_fields['id']!r} has a href {asset.href!r}."

Implement open_data and support tiff, netcdf and zarr for s3 and https protocol; Implement stack-mode which stacks items containing tiffs; #19

Implement open_data and support tiff, netcdf and zarr for s3 and https protocol; Implement stack-mode which stacks items containing tiffs; #19

Conversation

konstntokas commented Jul 31, 2024 • edited Loading

forman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forman commented Aug 5, 2024

konstntokas commented Aug 6, 2024

codecov bot commented Aug 6, 2024

Welcome to Codecov 🎉

konstntokas commented Aug 6, 2024

konstntokas commented Jul 31, 2024 •

edited

Loading