[Ready for review] Icechunk CLI Initial Implementation #716

DahnJ · 2025-02-11T00:31:20Z

Icechunk CLI Implementation

Initial implementation for #461

Design doc: #714

Implements

Python integration
Serializable configuration
Works against local filesystem and S3
Operations
- icechunk repo create
- icechunk snapshot list
- icechunk config init
- icechunk config add
- icechunk config list

paraseba

This is really great @DahnJ !

We don't need to wait until it's totally done to merge. lmk when you think you have reached a good milestone and we can merge in that state.

Cargo.lock

icechunk-python/src/lib.rs

icechunk/src/cli/config.rs

DahnJ · 2025-02-15T10:18:34Z

I've added a new version with serializable configuration. This works for me with both the Python binary and Rust one.

Make sure to edit the bucket in default.yaml directly or through editing default.rs which generates it.

cd icechunk
cargo install --path .

# Local repo
❯ icechunk repo create localrepo
✅ Created repository RepositoryAlias("localrepo")
❯ icechunk snapshot list localrepo
SnapshotInfo { id: 14883a60e82269cadf100ec8, parent_id: None, flushed_at: 2025-02-15T10:10:15.952248Z, message: "Repository initialized", metadata: {"__root": Bool(true)} }


# S3 repo
❯ icechunk repo create s3repo
✅ Created repository RepositoryAlias("s3repo")
❯ icechunk snapshot list s3repo
SnapshotInfo { id: 1e075c18c0c4e81ff2fb48e1, parent_id: None, flushed_at: 2025-02-15T10:09:43.722002Z, message: "Repository initialized", metadata: {"__root": Bool(true)} }

It's still WIP, but wanted to check that I'm going in the right direction with the config structure here.

paraseba

Looking great @DahnJ !!!!!!

paraseba · 2025-02-17T01:22:46Z

icechunk/default.yaml

@@ -0,0 +1,31 @@
+repos:


can we move this file src/bin/icechunk?

I have now removed the file altogether (5b03fbd), since it can be generated by icechunk config init

Should we instead decide on a standard location for the config, not relative to the repo? How about $HOME/.config/icechunk/cli_config.yaml, obtained through dirs::config_dir()?

Did that here 8f8d89f

On Mac, this actually defaults to $HOME/Library/Application Support.

paraseba · 2025-02-17T01:24:22Z

icechunk/src/bin/icechunk/main.rs

+#[tokio::main]
+async fn main() {
+    let cli = IcechunkCLI::parse();
+    let _ = run_cli(cli).await;


This should probably return an error, something like:

fn main() -> Result<(), Box<dyn Error>>

we will want the status code to be != 0 if some error happened

Improved error handling in 1724fcd

paraseba · 2025-02-17T01:26:32Z

icechunk/src/cli/config.rs

+
+use crate::config::{RepositoryConfig, S3Options, S3StaticCredentials};
+
+// Redefine to remove the Refreshable field


It seems like quite a bit of work just to remove that field. Remember that you'll need also to do the same for GCP and other credential types. Can we live with Refreshable even if we fail on it?

Initially I wanted to but the Refreshable field contains a trait object for which I couldn't simply derive PartialEq, preventing me from testing these configs. Having this subset-enum seemed like the simplest solution. Do you think there's a better one?

Yeah, you lose PartialEq but it may be the better than maintaining another parallel hierarchy.

We do a lot of assert!(matches!(...)) in tests when we cannot use PartialEq

Ah, nice, I didn't know about assert!(matches!(...)). Changed in 7cdd087

paraseba · 2025-02-17T01:28:46Z

icechunk/src/cli/default.rs

+        repos.repos.insert(RepositoryAlias("s3repo".to_string()), s3_repo);
+        repos.repos.insert(RepositoryAlias("localrepo".to_string()), local_repo);
+
+        let path = "default.yaml";


I don't know ... this test is writing code to the repos itself. Maybe move it to a temp file?

Absolutely, was just a quick way of generating something. The file is now removed.

I have added a rudimentary config init in 4d7afda

icechunk/src/cli/interface.rs

paraseba · 2025-02-17T01:32:42Z

icechunk/src/cli/interface.rs

+        repository.lookup_branch("main").await.context("❌ Failed to lookup branch")?;
+    let ancestry = repository.ancestry(&VersionInfo::SnapshotId(snapshot_id)).await?;
+    pin!(ancestry);
+    while let Some(snapshot_info) = ancestry.next().await {


navigating ancestry in Icechunk is (currently) very slow. I'd recommend putting a limit of say 10 parents, and then let users override that default.

You may want to look at the TryStreamExt trait for useful methods in Stream, so you don't have to code the explicit loop. There is try_map, take and all what you would expect from an iterator type.

Is this along the lines of what you were suggesting? 18d4d08

Great, and probably you no longer need to pin it

paraseba · 2025-02-17T01:33:28Z

icechunk/src/cli/interface.rs

+    let ancestry = repository.ancestry(&VersionInfo::SnapshotId(snapshot_id)).await?;
+    pin!(ancestry);
+    while let Some(snapshot_info) = ancestry.next().await {
+        println!("{:?}", snapshot_info?);


I recommend printing object ids (like SnapshotId) using its to_string method, instead of the Debug instance. It has a nicer, shorter presentation, and it's the characters that go into the object store path.

Wouldn't that require (in this case) SnapshotInfo to implement Display?

We probably should implement __repr__ for PySnapshotInfo

paraseba · 2025-02-17T01:44:11Z

@DahnJ one idea to think about: it could be good to write all the logic in the CLI as a library crate in the project, and then have the binary only dispatching to the library using clap. That way, we can test the CLi very easily, and other libraries can reuse it in the future.

…g init

DahnJ · 2025-02-18T23:30:02Z

Implemented a rudimentary interactive config init and config list

❯ icechunk config init
Enter alias: s3repo
Select repository type: S3
Enter bucket: mybucket
Enter prefix: myprefix
Enter region: eu-west-1
❯ icechunk config list
repos:
  s3repo: !S3
    location:
      bucket: mybucket
      prefix: myprefix
    object_store_config:
      region: eu-west-1
      endpoint_url: null
      anonymous: false
      allow_http: false
    credentials: FromEnv
    config:
      inline_chunk_threshold_bytes: null
      unsafe_overwrite_refs: null
      get_partial_values_concurrency: null
      compression: null
      caching: null
      storage: null
      virtual_chunk_containers: null
      manifest: null

paraseba

this is great!

paraseba · 2025-02-20T01:02:21Z

icechunk/Cargo.toml

@@ -55,6 +55,9 @@ tracing-subscriber = { version = "0.3.19", features = [
 tracing = "0.1.41"
 err-into = "1.0.1"
 serde_yaml_ng = "0.10.0"
+clap = { version = "4.5", features = ["derive"] }
+anyhow = "1.0.95"
+dialoguer = "0.11.0"


What would be the impact of creating another library crate in the workspace and putting everything cli related there? Projects that want to depend on Icechunk as a Rust library, wouldn't need to bring in all these dependencies.

How about declaring cli as an optional dependency, not installed by default? That could be more lightweight mechanism to provide a choice to the users.

I did this in 38747c1

This works for me both with cargo with the Rust library and maturin develop. Both require --features cli to install the CLI. Without it

Cargo won't install the executable at all

Maturin/Python will still add the icechunk executable that only prints

❯ icechunk Must install the optional `cli` feature to use the Icechunk CLI.

DahnJ · 2025-02-26T00:07:53Z

@DahnJ one idea to think about: it could be good to write all the logic in the CLI as a library crate in the project, and then have the binary only dispatching to the library using clap. That way, we can test the CLi very easily, and other libraries can reuse it in the future.

Regarding library crate, I offered an alternative in #716 (comment), let me please know what you think.

Regarding testing, made the interface more testable + added tests in 3137ace.

Sadly the dialoguer functions might not be easily testable.
We could also consider integration tests via assert_cmd but I think just testing the functions is sufficient for now

DahnJ · 2025-02-26T00:19:57Z

@paraseba as far as I am concerned, this is near a mergeable state. My goal with this PR is to provide something well-designed enough for others to be able to start contributing. Happy to make it good enough for Icechunk's standards though, let me know what you think.

dcherian · 2025-02-26T16:54:01Z

Thanks @DahnJ . Seba is on vacation this week, so feedback here will be a bit delayed.

Thanks for your patience.

DahnJ added 5 commits February 10, 2025 20:46

refactor(cli): directory

ad4847b

feat(cli): design doc + config

e166f1e

docs: remove design doc

a819028

Merge branch 'main' into feat/dj-cli-start

d3ec0da

feat(cli): follow proposed structure

a2778a1

This was referenced Feb 11, 2025

[Draft] Icechunk CLI Design Document #714

Draft

Icechunk could use a command line interface #461

Open

Merge branch 'main' into feat/dj-cli-start

6236b0a

paraseba reviewed Feb 11, 2025

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

Cargo.lock Outdated Show resolved Hide resolved

icechunk-python/src/lib.rs Show resolved Hide resolved

icechunk/src/cli/config.rs Outdated Show resolved Hide resolved

DahnJ added 6 commits February 11, 2025 23:41

chore: sync Cargo.lock back with main

1dbd8b5

style: whitespace

3f31866

feat(cli): CLI with a config

5b6c181

refactor(cli): add TODOs + minor refactors

449d984

refactor(cli): better RepositoryDefinition structure

3acf5d5

docs: typo

94941dd

DahnJ changed the title ~~[Draft] Icechunk CLI Implementation Skeleton~~ [Draft] Icechunk CLI Initial Implementation Feb 15, 2025

DahnJ added 2 commits February 15, 2025 14:14

refactor: bin name cli -> icechunk

7c79219

feat(cli): design-doc-aligned interface

691dd8c

paraseba reviewed Feb 17, 2025

View reviewed changes

DahnJ added 4 commits February 18, 2025 23:12

feat(cli): Ancestry with snapshot number limit

18d4d08

feat(cli): config init + config list

4d7afda

refactor(cli): remove default config - can be generated through confi…

5b03fbd

…g init

fix(cli): remove module, debug instead of display

6d096e3

refactor(cli): add a tood

c464462

paraseba reviewed Feb 20, 2025

View reviewed changes

DahnJ added 2 commits February 23, 2025 00:33

refactor(cli): Use existing S3Credentials enum

7cdd087

refactor(cli): Remove unnecessary pinning

1c857e6

feat(cli): Save to OS-dependent config dir

8f8d89f

DahnJ force-pushed the feat/dj-cli-start branch from 2f94e38 to 3bff618 Compare February 23, 2025 01:33

build(cli): CLI as optional dependency

38747c1

DahnJ force-pushed the feat/dj-cli-start branch from 3bff618 to 38747c1 Compare February 23, 2025 01:36

DahnJ added 6 commits February 23, 2025 11:43

refactor(cli): More readable user input matching

a02365b

feat(cli): No Python tracebacks + cleaner error handling

1724fcd

feat(cli): Add Tigris, Gcs, Azure

4ac073b

refactor(cli): Command functions accept config as an argument

2593843

feat(cli): Cleaner API + help messages

6feea11

refactor(cli): Testable CLI functions + tests

3137ace

DahnJ marked this pull request as ready for review February 26, 2025 00:19

DahnJ changed the title ~~[Draft] Icechunk CLI Initial Implementation~~ [Ready for review] Icechunk CLI Initial Implementation Feb 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ready for review] Icechunk CLI Initial Implementation #716

[Ready for review] Icechunk CLI Initial Implementation #716

DahnJ commented Feb 11, 2025 •

edited

Loading

paraseba left a comment

DahnJ commented Feb 15, 2025 •

edited

Loading

paraseba left a comment

paraseba Feb 17, 2025

DahnJ Feb 18, 2025 •

edited

Loading

paraseba Feb 20, 2025

DahnJ Feb 23, 2025

paraseba Feb 17, 2025

DahnJ Feb 24, 2025

paraseba Feb 17, 2025

DahnJ Feb 18, 2025

paraseba Feb 20, 2025

DahnJ Feb 23, 2025

paraseba Feb 17, 2025

DahnJ Feb 18, 2025 •

edited

Loading

paraseba Feb 17, 2025

DahnJ Feb 18, 2025

paraseba Feb 20, 2025

DahnJ Feb 23, 2025

paraseba Feb 17, 2025

DahnJ Feb 18, 2025

paraseba Feb 20, 2025

paraseba commented Feb 17, 2025

DahnJ commented Feb 18, 2025

paraseba left a comment

paraseba Feb 20, 2025

DahnJ Feb 23, 2025 •

edited

Loading

DahnJ commented Feb 26, 2025 •

edited

Loading

DahnJ commented Feb 26, 2025

dcherian commented Feb 26, 2025


		use crate::config::{RepositoryConfig, S3Options, S3StaticCredentials};

		// Redefine to remove the Refreshable field

[Ready for review] Icechunk CLI Initial Implementation #716

Are you sure you want to change the base?

[Ready for review] Icechunk CLI Initial Implementation #716

Conversation

DahnJ commented Feb 11, 2025 • edited Loading

Icechunk CLI Implementation

paraseba left a comment

Choose a reason for hiding this comment

DahnJ commented Feb 15, 2025 • edited Loading

paraseba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DahnJ Feb 18, 2025 • edited Loading

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

DahnJ Feb 18, 2025 • edited Loading

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

paraseba commented Feb 17, 2025

DahnJ commented Feb 18, 2025

paraseba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DahnJ Feb 23, 2025 • edited Loading

Choose a reason for hiding this comment

DahnJ commented Feb 26, 2025 • edited Loading

DahnJ commented Feb 26, 2025

dcherian commented Feb 26, 2025

DahnJ commented Feb 11, 2025 •

edited

Loading

DahnJ commented Feb 15, 2025 •

edited

Loading

DahnJ Feb 18, 2025 •

edited

Loading

DahnJ Feb 18, 2025 •

edited

Loading

DahnJ Feb 23, 2025 •

edited

Loading

DahnJ commented Feb 26, 2025 •

edited

Loading