Design doc for refactoring python API namespaces #1179

TomNicholas · 2025-08-18T20:23:46Z

Towards #1172

design-docs/012-python-namespaces

paraseba · 2025-08-19T13:43:03Z

design-docs/012-python-namespaces

+- `icechunk.Repository` (also available in `icechunk.repository.Repository`)
+- `icechunk.IcechunkStore` (also available in `icechunk.store.IcechunkStore`)
+- `icechunk.spec_version`
+- `icechunk.print_debug_info`


set_logs_filter should probably be at the top level

Should initialize_logs also go in the top-level?

paraseba · 2025-08-19T13:43:35Z

design-docs/012-python-namespaces

+  - `conflict.VersionSelection`
+- `exceptions`
+  - `exceptions.ConflictError`
+  - `exceptions.IcechunkError`


maybe IcechunkError at the top level

I agree. I expect ConflictError to be used more regularly so it should be top-level

I think either all exceptions should be top-level or none of them

paraseba · 2025-08-19T13:44:16Z

design-docs/012-python-namespaces

+- `logging`
+  - `logging.initialize_logs`,
+  - `logging.set_logs_filter`,
+- `manifests`


All of these should probably go in the config module

You mean the manifest splitting classes? They also seem like they should be grouped together though... They could actually go even deeper, i.e. icechunk.config.manifests.ManifestConfig

paraseba · 2025-08-19T13:47:11Z

design-docs/012-python-namespaces

+  - If we only kept `Storage` we could get rid of a _lot_ of functions in favour of methods...
+  - Alternatively should base classes that aren't really meant to be called by the user (e.g. `Storage`) be public API at all?
+- What's the difference between `storage.local_filesystem_storage` and `storage.local_filesystem_store`?
+  - Similarly how can we better disambiguate `storage.s3_storage` and `storage.s3_store`?


we probably need better names. Storage is what you pass to create/open repos. s3_store is a way to indicate the target of a VirtualChunkContainer

Okay yeah that's really confusing

I sketched an alternative proposal in #1182

paraseba · 2025-08-19T13:47:26Z

design-docs/012-python-namespaces

+  - Alternatively should base classes that aren't really meant to be called by the user (e.g. `Storage`) be public API at all?
+- What's the difference between `storage.local_filesystem_storage` and `storage.local_filesystem_store`?
+  - Similarly how can we better disambiguate `storage.s3_storage` and `storage.s3_store`?
+- Is there a similar question for credentials?


Credentials are currently a mess

These deserve their own separate issue. The class hierarchy is complicated so it's not immediately obvious to me how to simplify the API.

paraseba · 2025-08-19T13:48:32Z

design-docs/012-python-namespaces

+  - Similarly should `S3Options` really be `S3Config`?
+- Should summary dataclasses such as `Diff` and `GCSummary` live in the top-level?
+- Is `distributed` meant to be a public module at all? Why is it's API defined in a different place? Should it be merged with `dask`?
+- Union types like `AnyCredential` are re-exported as top-level API for importing, but it seems like they are only useful for internal type-hinting purposes?


Only for typing, but not sure if they are needed publicly or only privately

If they are only needed for typing, and you don't expect users to make great use of them for typing, I think they should be private.

paraseba · 2025-08-19T13:49:15Z

design-docs/012-python-namespaces

+
+2. **Move everything into desired locations whilst keeping all top-level imports**
+
+We should add tests that both import paths work.


Yes please, particularly we need to add tests that verify we are not breaking user code

paraseba · 2025-08-19T13:50:43Z

design-docs/012-python-namespaces

+User experience should look like this:
+
+```python
+# works, but emits a warning


I think we should use Icechunk 2 as the moment to make the change and start warning people about deprecation.

design-docs/012-python-namespaces

TomNicholas · 2025-08-21T15:33:12Z

@paraseba there are still a few unresolved question in this design doc, when you have a moment.

Anu-Ra-g · 2025-08-21T16:26:22Z

design-docs/012-python-namespaces

+  - `distributed.extract_sessions`
+  - `distributed.merge_sessions`
+- `garbage`
+  - `garbage.GCSummary`


There is no such module garbage in icechunk!!

Oh yes you're right. Let's make one then. I'll edit this design doc to move garbage into another section.

Anu-Ra-g · 2025-08-21T16:40:12Z

design-docs/012-python-namespaces

+Existing namespaces, with new members added:
+
+- `credentials`
+  - `credentials.Credentials`


As per this doc, the classes and functions from the existing sub-modules would be exported from the module itself using __all__ variable instead of the top-level icechunk package?

For now, we actually want both: exported from the module itself, and then re-imported from the sub-module into the top-level __init__.py. That way your initial PR doesn't change the user experience, but does define a clearer underlying structure of sub-modules. So your current PR is for step 2 as described at the end of this doc.

So for now only add the new namespaces to icechunk?

We currently have this:

# __init__.py from icechunk._icechunk_python import Credentials __all__ = [ "Credentials", ]

# icechunk.icechunk_python.pyi class Credentials ...

Which only allows you to import via from icechunk import Credentials.

What we want is this:

# __init__.py from icechunk.credentials import Credentials __all__ = [ "Credentials", ]

# icechunk.credentials.py from icechunk._icechunk_python import Credentials __all__ = [ "Credentials", ]

# icechunk_python.pyi class Credentials ...

This would enable importing via from icechunk.credentials import Credentials (i.e. the end state we want) whilst for now still allowing importing via from icechunk import Credentials (i.e. backwards compatibility), and requires no changes to the part of the python code that interacts with rust (i.e. separates API from implementation).

draft design doc

8e2abfc

TomNicholas added python 🐍 Related to python code API design 🎨 design doc 🧑‍🎨 labels Aug 18, 2025

TomNicholas temporarily deployed to true August 18, 2025 20:23 — with GitHub Actions Inactive

TomNicholas mentioned this pull request Aug 18, 2025

Split up top-level python namespace into modules #1172

Open

TomNicholas temporarily deployed to true August 18, 2025 20:30 — with GitHub Actions Inactive

paraseba reviewed Aug 19, 2025

View reviewed changes

TomNicholas commented Aug 19, 2025

View reviewed changes

design-docs/012-python-namespaces Outdated Show resolved Hide resolved

Got the two example syntaxes the wrong way around

b51131f

TomNicholas temporarily deployed to true August 19, 2025 14:11 — with GitHub Actions Inactive

paraseba approved these changes Aug 20, 2025

View reviewed changes

remove IcechunkStore from top-level API

db4671c

TomNicholas temporarily deployed to true August 20, 2025 14:23 — with GitHub Actions Inactive

move logging to config module

21a3ddc

TomNicholas temporarily deployed to true August 20, 2025 15:26 — with GitHub Actions Inactive

TomNicholas mentioned this pull request Aug 20, 2025

Replace constructor functions with methods #1182

Open

remove AnyObjectStoreConfig type

40307ef

Anu-Ra-g mentioned this pull request Aug 21, 2025

Added new namespaces for icechunk #1189

Closed

Anu-Ra-g reviewed Aug 21, 2025

View reviewed changes

TomNicholas added 2 commits August 21, 2025 12:57

remove constructor functions, following idea in earth-mover#1182

bf5429f

move garbage to section for new namespaces

063c59d


		2. Move everything into desired locations whilst keeping all top-level imports

		We should add tests that both import paths work.

Design doc for refactoring python API namespaces #1179

Are you sure you want to change the base?

Design doc for refactoring python API namespaces #1179

Uh oh!

Conversation

TomNicholas commented Aug 18, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcherian Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TomNicholas commented Aug 21, 2025

Uh oh!

Anu-Ra-g Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dcherian Aug 20, 2025 •

edited

Loading

Anu-Ra-g Aug 21, 2025 •

edited

Loading