feat(client): implement ClientOperationCfg to control any client oper… #2657

RolandSherwin · 2025-01-22T19:14:15Z

No description provided.

grumbach

Good work! Couple of areas where I have doubts and questions.

ant-cli/src/commands/file.rs

autonomi/src/client/config.rs

autonomi/src/client/data_types/chunk.rs

autonomi/src/client/data_types/graph.rs

autonomi/src/client/data_types/pointer.rs

grumbach · 2025-01-31T03:14:38Z

ant-node/src/put_validation.rs

-        // loud mode: print a celebratory message to console
-        #[cfg(feature = "loud")]
-        {
-            println!("🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟   RECEIVED REWARD   🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟");
-            println!(
-                "Total payment of {reward_amount:?} atto tokens accepted for record {pretty_key}"
-            );
-            println!("🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟🌟");
-        }
-


Wait this is useful, please keep it

grumbach · 2025-01-31T03:16:40Z

ant-cli/src/actions/download.rs

+    if json {
+        download_output.print_json();
+    } else {
+        download_output.print();
+    }


Can't this be part of client config so we don't need to carry around a json bool?

grumbach · 2025-01-31T03:20:26Z

autonomi/Cargo.toml

@@ -23,7 +23,6 @@ name = "put_and_dir_upload"
 default = []
 external-signer = ["ant-evm/external-signer"]
 extension-module = ["pyo3/extension-module"]
-loud = []


This feature is very useful for local testing for us, I think we should keep it around. Most users will never us it but that's fine, it's off by default.

grumbach · 2025-01-31T03:21:36Z

autonomi/src/client/high_level/files/archive_public.rs

-        #[cfg(feature = "loud")]
-        println!(
-            "Uploading public archive referencing {} files",
-            archive.map().len()
-        );
-


Although I agree it's not the prettiest way to do it, it's an excellent UX for CLI. Until we find another way I think we need to keep it

grumbach · 2025-01-31T03:21:46Z

autonomi/src/client/high_level/files/fs_private.rs

-        #[cfg(feature = "loud")]
-        println!("Upload completed in {:?}", start.elapsed());


grumbach · 2025-01-31T03:21:52Z

autonomi/src/client/high_level/files/fs_private.rs

-        #[cfg(feature = "loud")]
-        println!("Uploading file: {path:?}");


autonomi/src/client/mod.rs

grumbach · 2025-01-31T03:23:36Z

autonomi/src/client/mod.rs

-                            .expect("receiver should not close");
+                            .expect("Could not send TimedOutWithIncompatibleProtocol, receiver should not close");
                    } else {
                        sender
                            .send(Err(ConnectError::TimedOut))
-                            .expect("receiver should not close");
+                            .expect("Could not send TimedOut, receiver should not close");
                    }
                }
            }
            event = event_receiver.recv() => {
-                let event = event.expect("receiver should not close");
+                let event = event.expect("Could not receiver Network event, receiver should not close");


Although this originates not in this PR, I suggest we BAN expect altogether as they do fail sometimes!

grumbach · 2025-01-31T03:25:27Z

ant-cli/Cargo.toml

@@ -27,7 +27,7 @@ ant-bootstrap = { path = "../ant-bootstrap", version = "0.1.4" }
 ant-build-info = { path = "../ant-build-info", version = "0.1.23" }
 ant-logging = { path = "../ant-logging", version = "0.2.45" }
 ant-protocol = { path = "../ant-protocol", version = "0.3.3" }
-autonomi = { path = "../autonomi", version = "0.3.5", features = [ "loud" ] }


This feature although I agree not the most elegant way to do it, made the UX great. Removing it will result in a frustrating user experience while waiting for uploads.

Also not a proponent of removing this. Users on the forum are already showing a lack of understanding what's happening when using the CLI.

grumbach · 2025-01-31T03:28:29Z

ant-networking/src/event/mod.rs

-        #[cfg(feature = "loud")]
-        println!("Estimated network size: {estimated_network_size:?}");


This too is very useful for local tests

grumbach · 2025-01-31T03:28:36Z

ant-networking/src/event/mod.rs

-        #[cfg(feature = "loud")]
-        println!("New peer added to routing table: {added_peer:?}, now we have #{n_peers} connected peers");
-


grumbach · 2025-01-31T03:28:48Z

ant-networking/src/event/swarm.rs

-                                    #[cfg(feature = "loud")]
-                                    {
-                                        println!("Multiaddr not supported : {addr:?}");
-                                        println!("If this was your bootstrap peer, restart your node with a supported multiaddr");
-                                    }


This as well

b-zee · 2025-02-03T08:02:18Z

ant-cli/Cargo.toml

@@ -27,7 +27,7 @@ ant-bootstrap = { path = "../ant-bootstrap", version = "0.1.4" }
 ant-build-info = { path = "../ant-build-info", version = "0.1.23" }
 ant-logging = { path = "../ant-logging", version = "0.2.45" }
 ant-protocol = { path = "../ant-protocol", version = "0.3.3" }
-autonomi = { path = "../autonomi", version = "0.3.5", features = [ "loud" ] }


Also not a proponent of removing this. Users on the forum are already showing a lack of understanding what's happening when using the CLI.

b-zee · 2025-02-03T09:03:35Z

ant-cli/src/actions/download.rs

        .await
-        .wrap_err("Failed to fetch data from address")?;
+        .map_err(|err| {
+            error!("Failed to fetch archive from address: {private_address:?}");


Use inspect_err to report errors.

We actually map the error to a different type here, so it should be fine for the logging to go in here I think.

b-zee · 2025-02-03T09:03:45Z

ant-cli/src/actions/download.rs

-        std::fs::write(path, bytes)?;
-        progress_bar.clone().inc(1);
+        std::fs::create_dir_all(parent).map_err(|err| {
+            error!("Failed to create parent directories for {path:?}: {err}");


Use inspect_err

Same as above, we actually map the error to a different type here, so it should be fine for the logging to go in here.

b-zee · 2025-02-03T09:03:52Z

ant-cli/src/actions/download.rs

+            (err.into(), all_errs.clone())
+        })?;
+        std::fs::write(&path, bytes).map_err(|err| {
+            error!("Failed to write file {path:?}: {err}");


Use inspect_err

Same as above.

b-zee · 2025-02-03T09:04:02Z

ant-cli/src/actions/download.rs

-        .wrap_err("Failed to fetch data from address")?;
+) -> Result<(), (DownloadError, Vec<(String, String)>)> {
+    let archive = client.archive_get_public(address).await.map_err(|err| {
+        error!("Failed to fetch archive from address: {address:?}");


Use inspect_err.

b-zee · 2025-02-03T09:04:11Z

ant-cli/src/actions/download.rs

-        std::fs::write(path, bytes)?;
-        progress_bar.clone().inc(1);
+        std::fs::create_dir_all(parent).map_err(|err| {
+            error!("Failed to create parent directories for {path:?}: {err}");


Use inspect_err.

b-zee · 2025-02-03T09:04:31Z

ant-cli/src/actions/download.rs

+            (err.into(), all_errs.clone())
+        })?;
+        std::fs::write(&path, bytes).map_err(|err| {
+            error!("Failed to write file {path:?}: {err}");


Use inspect_err.

…ation

RolandSherwin force-pushed the uploader_metrics branch 3 times, most recently from 9be3586 to 33b97f4 Compare January 23, 2025 18:57

b-zee approved these changes Jan 27, 2025

View reviewed changes

RolandSherwin force-pushed the uploader_metrics branch 2 times, most recently from 30e7793 to 1b159fe Compare January 29, 2025 15:46

maqi approved these changes Jan 29, 2025

View reviewed changes

grumbach requested changes Jan 29, 2025

View reviewed changes

grumbach self-requested a review January 31, 2025 02:28

grumbach approved these changes Jan 31, 2025

View reviewed changes

grumbach requested changes Jan 31, 2025

View reviewed changes

grumbach reviewed Jan 31, 2025

View reviewed changes

autonomi/src/client/mod.rs Outdated Show resolved Hide resolved

grumbach reviewed Jan 31, 2025

View reviewed changes

maqi mentioned this pull request Jan 31, 2025

[feature] APIs which charge should allow price limiting #2048

Open

b-zee requested changes Feb 3, 2025

View reviewed changes

RolandSherwin added 6 commits February 3, 2025 20:23

feat(client): implement ClientOperationCfg to control any client oper…

afeb194

…ation

feat: add cli json outupt, add more client events

466efef

feat(build_info): implement VersionInfo struct for easier re-use

625b146

chore: more accurate error msgs during expect

74f55d9

refactor(client): de duplicate client event emit code

1df9a6d

fix(client): remove random print statements and fix json output values

5231a91

RolandSherwin force-pushed the uploader_metrics branch from b87c63a to 5231a91 Compare February 3, 2025 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client): implement ClientOperationCfg to control any client oper… #2657

feat(client): implement ClientOperationCfg to control any client oper… #2657

RolandSherwin commented Jan 22, 2025

grumbach left a comment

grumbach Jan 31, 2025

grumbach Jan 31, 2025

grumbach Jan 31, 2025

grumbach Jan 31, 2025

grumbach Jan 31, 2025

grumbach Jan 31, 2025

grumbach Jan 31, 2025

grumbach Jan 31, 2025

b-zee Feb 3, 2025

grumbach Jan 31, 2025

grumbach Jan 31, 2025

grumbach Jan 31, 2025

b-zee Feb 3, 2025

b-zee Feb 3, 2025

RolandSherwin Feb 3, 2025

b-zee Feb 3, 2025

RolandSherwin Feb 3, 2025

b-zee Feb 3, 2025

RolandSherwin Feb 3, 2025

b-zee Feb 3, 2025

b-zee Feb 3, 2025

b-zee Feb 3, 2025

		#[cfg(feature = "loud")]
		println!("Upload completed in {:?}", start.elapsed());

		#[cfg(feature = "loud")]
		println!("Uploading file: {path:?}");

		#[cfg(feature = "loud")]
		println!("Estimated network size: {estimated_network_size:?}");

		#[cfg(feature = "loud")]
		println!("New peer added to routing table: {added_peer:?}, now we have #{n_peers} connected peers");

feat(client): implement ClientOperationCfg to control any client oper… #2657

Are you sure you want to change the base?

feat(client): implement ClientOperationCfg to control any client oper… #2657

Conversation

RolandSherwin commented Jan 22, 2025

grumbach 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment