Refactoring Preload API #12

b-yogesh · 2024-12-30T16:34:03Z

This PR builds on top of #6

Changes in this PR:

Extend ExecutorPreloadHandle from xcube for preload operations in the CLMS data store
Improve tests and readability
Use pytest-vcr to send actual requests and records it as cassettes to test them like real API responses.
Modify CI to install xcube deps
Update README.md
Update CHANGES.md
Refactoring the codebase

Closes #10
Closes #9
Closes #8
Closes #4
Closes #3

…oad_request

konstntokas

I already looks a lot more structured and it is easier to read than in the previous PR. :)

Please view my comments below.

CHANGES.md

environment.yml

pyproject.toml

examples/notebooks/CLMSDataStoreTutorial.ipynb

xcube_clms/constants.py

xcube_clms/store.py

xcube_clms/clms.py

xcube_clms/utils.py

…ls the preload

konstntokas

The key point is that the store should clearly reflect that all (preloaded) datasets are stored as Zarr files. Please double-check to ensure my assumptions about this are correct and make any necessary adjustments. I'll review it briefly later today, and then I’ll approve.

The remaining comments are only cosmetics.

konstntokas · 2025-01-17T08:12:33Z

test/test_store.py

+from xcube.util.jsonschema import (
+    JsonObjectSchema,
+)


Suggested change

from xcube.util.jsonschema import (

JsonObjectSchema,

)

from xcube.util.jsonschema import JsonObjectSchema

konstntokas · 2025-01-17T08:13:08Z

test/test_store.py

@@ -25,9 +25,11 @@
 import pytest
 import xarray as xr
 from xcube.core.store import new_data_store, DatasetDescriptor


Suggested change

from xcube.core.store import new_data_store, DatasetDescriptor

from xcube.core.store import new_data_store

from xcube.core.store import DatasetDescriptor

I'd prefer single imports here.

Yes, xcube coding convention says so.

konstntokas · 2025-01-17T08:13:31Z

test/test_store.py


-from xcube_clms.constants import DATA_STORE_ID, DATA_ID_SEPARATOR, DATA_OPENER_IDS
+from xcube_clms.constants import DATA_STORE_ID, DATA_ID_SEPARATOR


Suggested change

from xcube_clms.constants import DATA_STORE_ID, DATA_ID_SEPARATOR

from xcube_clms.constants import DATA_STORE_ID

from xcube_clms.constants import DATA_ID_SEPARATOR

I'd prefer single line imports.

test/test_store.py

konstntokas · 2025-01-17T08:19:17Z

test/test_store.py

        self.assertIsInstance(schema, JsonObjectSchema)
-        self.assertEqual({}, schema.properties)
+        self.assertEqual(


This will need to be adjusted then, since you get the opener params for zarr.

xcube_clms/clms.py

xcube_clms/preload.py

xcube_clms/store.py

test/test_preload.py

forman · 2025-01-17T09:28:11Z

test/test_store.py

@@ -25,9 +25,11 @@
 import pytest
 import xarray as xr
 from xcube.core.store import new_data_store, DatasetDescriptor


Yes, xcube coding convention says so.

xcube_clms/clms.py

xcube_clms/processor.py

xcube_clms/store.py

xcube_clms/download_manager.py

xcube_clms/constants.py

xcube_clms/clms.py

forman · 2025-01-17T11:10:57Z

xcube_clms/clms.py

+        """Retrieves all data IDs, optionally including additional attributes.
+
+        Args:
+            include_attrs: Specifies whether to include attributes.


Right, but we should also do it. Could you please create an xcube issue for that and maybe a PR as well. Should be < 1h.

xcube_clms/store.py

forman

Good job, very clean code, thanks!

xcube_clms/utils.py

konstntokas

Nice, approved!

b-yogesh added 30 commits November 4, 2024 20:54

Implement initial methods in clms.py

4082cab

Refactor code

a55d133

Refactor code again

52b1e2a

Implement get_data_ids generator

31ccd70

Tiny typo fix

b124ef1

Implement has_data

39d8b37

Initial version of describe_data (not working entirely)

3b34c3a

Implement describe_data

70a0456

Implement get_open_data_params_schema

c075479

Implemented access token class

49cd69b

Implemented access token class

4ffa9ab

[In progress] - open_data implementation

f8ccbfb

[In progress] - open_data implementation - implemented _prepare_downl…

94ff10d

…oad_request

[In progress] - open_data implementation - more impl.

26a6322

[In progress] - open_data implementation - more impl.

f6645f2

Update make_api_request

7951b2e

temporary bbox and crs handling

e950bb7

fix condition

5ccede3

fix get_data_store_params_schema

24e93d1

Add constants

2e5f46f

Refactoring

fc938bc

Add schema for preload_data

76169d4

Remove error message truncation

d680129

Update get_metadata method

a91bd5c

Add initial unsupported datasets check

8aa6014

Raise exceptions for unsupported data and add preload params schema

13febb6

Update schema methods

63ba00c

Add TODOs

a049218

Implement first TODO: change data_id def

a6ccac0

Implement first TODO: add include_attr bool impl

3a1c329

b-yogesh mentioned this pull request Jan 13, 2025

First version implementation of preload_data and the rest of CLMS data store #6

Closed

konstntokas requested changes Jan 13, 2025

View reviewed changes

b-yogesh added 5 commits January 15, 2025 17:16

Update based on reviewer's comments

d5e50b5

Update based on reviewer's comments, pt. 2

15fe1ea

Add missing cassette

0173b25

Update notebook + use protocol for data opener ids

3aa8cfa

Update for better notifications, and removing close() when user cance…

c8994d2

…ls the preload

b-yogesh requested a review from konstntokas January 16, 2025 16:01

konstntokas requested changes Jan 17, 2025

View reviewed changes

forman requested changes Jan 17, 2025

View reviewed changes

b-yogesh added 14 commits January 17, 2025 13:08

Multiple imports -> single imports per line

1a2914d

update based on reviewers comments

268f2ff

Remove tqdm

4eeb990

add dynamic chunk sizes

51504a4

Update get_data_ids method args type

9edb5e4

Fix tests

bc484f9

Update notebook

702003a

Remove redundant method

b1a5241

Add spaces after the header

14946d2

Update notebook

39bd4bf

Update notebook

5f61c45

Remove tqdm dep

d02292f

Update CHANGES.md

4f8f885

Update CHANGES.md

cfda06c

b-yogesh requested review from konstntokas and forman January 20, 2025 16:22

forman approved these changes Jan 21, 2025

View reviewed changes

xcube_clms/utils.py Show resolved Hide resolved

konstntokas approved these changes Jan 21, 2025

View reviewed changes

b-yogesh merged commit b5606e4 into main Jan 21, 2025
3 checks passed

b-yogesh deleted the yogesh_preload-data-2 branch January 21, 2025 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring Preload API #12

Refactoring Preload API #12

b-yogesh commented Dec 30, 2024 •

edited

Loading

konstntokas left a comment •

edited

Loading

konstntokas left a comment

konstntokas Jan 17, 2025

konstntokas Jan 17, 2025

konstntokas Jan 17, 2025

forman Jan 17, 2025

konstntokas Jan 17, 2025

konstntokas Jan 17, 2025

konstntokas Jan 17, 2025

forman Jan 17, 2025

forman Jan 17, 2025

forman left a comment

konstntokas left a comment

	from xcube.core.store import new_data_store, DatasetDescriptor
	from xcube.core.store import new_data_store
	from xcube.core.store import DatasetDescriptor


		from xcube_clms.constants import DATA_STORE_ID, DATA_ID_SEPARATOR, DATA_OPENER_IDS
		from xcube_clms.constants import DATA_STORE_ID, DATA_ID_SEPARATOR

	from xcube_clms.constants import DATA_STORE_ID, DATA_ID_SEPARATOR
	from xcube_clms.constants import DATA_STORE_ID
	from xcube_clms.constants import DATA_ID_SEPARATOR

Refactoring Preload API #12

Refactoring Preload API #12

Conversation

b-yogesh commented Dec 30, 2024 • edited Loading

konstntokas left a comment • edited Loading

Choose a reason for hiding this comment

konstntokas 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

forman left a comment

Choose a reason for hiding this comment

konstntokas left a comment

Choose a reason for hiding this comment

b-yogesh commented Dec 30, 2024 •

edited

Loading

konstntokas left a comment •

edited

Loading