Improve cli args parsing in bench-tps #30307

KirillLykov · 2023-02-14T13:22:49Z

Problem

Parsing cli args is different from other places (due to exit, panic, println), it also makes it more difficult to test.
Added a basic unit test for cli.
This is preliminary work to migrate it to clap-v3, most of the things will be rewritten during migration to clap-v3

Summary of Changes

Rename parse function
Get rid of panics, exit and println
Add basic unit tests

KirillLykov · 2023-02-14T14:16:26Z

Rust compiler shows warnings which looks weird for me:

error: unused imports: `read_keypair_file`, `super::*`, `write_keypair_file`
   --> bench-tps/src/cli.rs:559:9
    |
559 |         super::*,
    |         ^^^^^^^^
560 |         solana_sdk::signature::{read_keypair_file, write_keypair_file, Keypair, Signer},
    |                                 ^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^
    |
    = note: `-D unused-imports` implied by `-D warnings`
 
    Checking solana-validator v1.16.0 (/solana/validator)
    Checking solana-rpc-test v1.16.0 (/solana/rpc-test)
    Checking solana-client-test v1.16.0 (/solana/client-test)
error: function `make_tmp_path` is never used
   --> bench-tps/src/cli.rs:563:8
    |
563 |     fn make_tmp_path(name: &str) -> String {
    |        ^^^^^^^^^^^^^
    |
    = note: `-D dead-code` implied by `-D warnings`

because these imports and function are clearly used

ilya-bobyr · 2023-02-14T19:25:12Z

Rust compiler shows warnings which looks weird for me:

[...]

because these imports and function are clearly used

This is because tests are not part of the normal compilation profile.
They are only compiled when the unit test binaries are generated.
It means, that this function:

    #[test]
    #[allow(clippy::cognitive_complexity)]
    fn test_cli_parse() {

is ignored by the compiler. The compiler will then see that

    fn make_tmp_path(name: &str) -> String {

is not called by anyone in the same module.
And as it has visibility that restricts it to the module, it can not be used by anybody else.

Adding #[cfg(test)] to the tests module definition makes the whole module only visible when building tests. And removes the warnings:

diff --git bench-tps/src/cli.rs bench-tps/src/cli.rs
index b6eb6..2956d 100644
--- bench-tps/src/cli.rs
+++ bench-tps/src/cli.rs
@@ -554,6 +554,7 @@ pub fn parse_args(matches: &ArgMatches) -> Result<Config, &'static str> {
     Ok(args)
 }

+#[cfg(test)]
 mod tests {
     use {
         super::*,

CriesofCarrots · 2023-02-14T21:57:01Z

bench-tps/src/cli.rs

+mod tests {
+    use {
+        super::*,
+        solana_sdk::signature::{read_keypair_file, write_keypair_file, Keypair, Signer},


Also, since you are using super::*, you don't need to reimport elements imported in the top-level module.

Suggested change

solana_sdk::signature::{read_keypair_file, write_keypair_file, Keypair, Signer},

solana_sdk::signature::{write_keypair_file, Signer},

The evil of the wildcard imports is raising its ugly head

Grumble. This is now inconsistent with the pattern in nearly every other test module in the repo, where we use super::*

Could you clarify what exactly is inconsistent?
After using super::* it looks similar to the other tests that use super::*.

But this one isn't using super::*

Sorry, I got confused.
Somehow, I saw this change as using super::*.
It is even in the email that GitHub sent:

GitHub reviewer experience is a rollercoaster %)

Lol, yeah, it's weird it's not showing that preview as outdated.
Oh well. I'm not going to fight on this one anymore anyway.

bench-tps/src/cli.rs

ilya-bobyr · 2023-02-14T22:28:49Z

bench-tps/src/cli.rs

+mod tests {
+    use {
+        super::*,
+        solana_sdk::signature::{read_keypair_file, write_keypair_file, Keypair, Signer},


The evil of the wildcard imports is raising its ugly head

bench-tps/src/cli.rs

bench-tps/src/main.rs

ilya-bobyr · 2023-02-15T05:53:26Z

bench-tps/src/cli.rs

@@ -35,12 +35,14 @@ impl Default for ExternalClientType {
    }
 }

+#[derive(PartialEq, Debug)]


minor

When deriving PartialEq, you normally also want to derive Eq.
Only a rather specific set of types (such as floating point numbers) implement only PartialEq without Eq.
But as the compiler can not check it, it is up to the person writing the code to make this call.

Here are the details on when absence of Eq matters: https://doc.rust-lang.org/book/appendix-03-derivable-traits.html?highlight=partialeq#partialeq-and-eq-for-equality-comparisons

Here the problem with Eq is that Keypair is not implementing it

Not sure why it is the case.
Keypair is just a PublicKey and a SecretKey.
Seems like it might be intentional, as PublicKey does implement it.
Not sure why SecretKey would not.
I've opened an issue, with a few questions, in the library repo.

But you can still add Eq to the other places where you are adding PartialEq.
At least, I personally, try to do it by default.
When someone needs it, they need to go and figure out if Eq was omitted by accident or not. Which is generally more complex, compared to adding 3 characters when the code is written :))

I just realized that PartialEq was implemented for Keypair in https://github.com/solana-labs/solana/blob/master/sdk/src/signer/keypair.rs#L107-L114
Seems like this is exactly the case when PartialEq was implemented, without adding Eq in there.

Here is a PR to fix it: #30381

Looks like leaving as it is

ilya-bobyr

Yay to the unit tests and proper error handling!

ilya-bobyr · 2023-02-17T00:22:43Z

bench-tps/src/cli.rs

@@ -35,12 +35,14 @@ impl Default for ExternalClientType {
    }
 }

+#[derive(PartialEq, Debug)]


Not sure why it is the case.
Keypair is just a PublicKey and a SecretKey.
Seems like it might be intentional, as PublicKey does implement it.
Not sure why SecretKey would not.
I've opened an issue, with a few questions, in the library repo.

But you can still add Eq to the other places where you are adding PartialEq.
At least, I personally, try to do it by default.
When someone needs it, they need to go and figure out if Eq was omitted by accident or not. Which is generally more complex, compared to adding 3 characters when the code is written :))

ilya-bobyr · 2023-02-17T00:27:52Z

bench-tps/src/cli.rs

+        let parsed_data_size = data_size
+            .to_string()
+            .parse()
+            .map_err(|_| "Can't parse padded instruction data size")?;
        args.instruction_padding_config = Some(InstructionPaddingConfig {
            program_id,
-            data_size: data_size
-                .to_string()
-                .parse()
-                .expect("Can't parse padded instruction data size"),
+            data_size: parsed_data_size,


<minor-style>

I think it is totaly fine to shadow data_size here:

let data_size = data_size .to_string() .parse() .map_err(|_| "Can't parse padded instruction data size")?; args.instruction_padding_config = Some(InstructionPaddingConfig { program_id, data_size, });

</minor-style>

By the way, .to_string() is unnecessary here.
.parse() works just fine on a &str.

CriesofCarrots · 2023-02-23T16:50:21Z

bench-tps/src/cli.rs

+mod tests {
+    use {
+        super::*,
+        solana_sdk::signature::{read_keypair_file, write_keypair_file, Keypair, Signer},


Grumble. This is now inconsistent with the pattern in nearly every other test module in the repo, where we use super::*

ilya-bobyr

A few minor comments, but looks good to me otherwise :)

ilya-bobyr · 2023-02-23T19:45:25Z

bench-tps/src/cli.rs

-            .parse()
-            .expect("can't parse tpu_connection_pool_size");
+            .parse::<usize>()
+            .map_err(|_| "can't parse tpu_connection_pool_size")?;


From the user standpoint, the argument name is tpu-connection-pool-size, with dashes, right?
Here and in a few other places.
Technically, this issue was there before. But, maybe, you would want to fix it as well, as you are doing this cleanup.

this is correct, so I've fixed it.
The plan is to eliminate this manual parsing by using clap-v3 with derive feature

ilya-bobyr · 2023-02-23T19:46:57Z

bench-tps/src/cli.rs

@@ -486,23 +485,25 @@ pub fn extract_args(matches: &ArgMatches) -> Config {
    }

    if let Some(v) = matches.value_of("target_lamports_per_signature") {
-        args.target_lamports_per_signature = v.to_string().parse().expect("can't parse lamports");
+        args.target_lamports_per_signature =
+            v.to_string().parse().map_err(|_| "can't parse lamports")?;


Suggested change

v.to_string().parse().map_err(|_| "can't parse lamports")?;

v.to_string().parse().map_err(|_| "can't parse target-lamports-per-signature")?;

ilya-bobyr · 2023-02-23T19:56:57Z

bench-tps/src/cli.rs

    args.target_node = matches
        .value_of("target_node")
-        .map(|target_str| target_str.parse().unwrap());
+        .map(|target_str| target_str.parse::<Pubkey>().unwrap());


This is still panicking. Here is how to fix it, to make it similar the other cases:

Suggested change

args.target_node = matches

.value_of("target_node")

.map(|target_str| target_str.parse().unwrap());

.map(|target_str| target_str.parse::<Pubkey>().unwrap());

args.target_node = matches

.value_of("target_node")

.map(|target_str| target_str.parse::<Pubkey>())

.transpose()

.map_err(|_| "Failed to parse target-node")?;

transpose() docs

ilya-bobyr · 2023-02-23T19:57:22Z

bench-tps/src/cli.rs


    if let Some(v) = matches.value_of("num_lamports_per_account") {
-        args.num_lamports_per_account = v.to_string().parse().expect("can't parse lamports");
+        args.num_lamports_per_account =
+            v.to_string().parse().map_err(|_| "can't parse lamports")?;


Suggested change

v.to_string().parse().map_err(|_| "can't parse lamports")?;

v.to_string().parse().map_err(|_| "can't parse num-lamports-per-account")?;

ilya-bobyr · 2023-02-23T19:57:42Z

bench-tps/src/cli.rs

    }

    if let Some(t) = matches.value_of("target_slots_per_epoch") {
        args.target_slots_per_epoch = t
            .to_string()
            .parse()
-            .expect("can't parse target slots per epoch");
+            .map_err(|_| "can't parse target slots per epoch")?;


Suggested change

.map_err(|_| "can't parse target slots per epoch")?;

.map_err(|_| "can't parse target-slots-per-epoch")?;

ilya-bobyr · 2023-02-23T19:58:08Z

bench-tps/src/cli.rs

-        );
+        let parsed_num_conflict_groups = num_conflict_groups
+            .parse()
+            .map_err(|_| "Can't parse conflict groups")?;


Suggested change

.map_err(|_| "Can't parse conflict groups")?;

.map_err(|_| "Can't parse num-conflict-groups")?;

ilya-bobyr · 2023-02-23T19:58:18Z

bench-tps/src/cli.rs

-            exit(1)
-        });
+        args.bind_address =
+            solana_net_utils::parse_host(addr).map_err(|_| "Failed to parse bind_address")?;


Suggested change

solana_net_utils::parse_host(addr).map_err(|_| "Failed to parse bind_address")?;

solana_net_utils::parse_host(addr).map_err(|_| "Failed to parse bind-address")?;

* Rename parse funciton * Get rid of panics, exit and println * Add basic unit tests

…t_id tests

* Improve cli args parsing in bench-tps * Rename parse funciton * Get rid of panics, exit and println * Add basic unit tests * add allow dead_code and unused_imports for cli tests * simplify code by using map_err instead of macro * simplify data_size parsing * use tempfile crate to create temp keypair files * fix bug with reading client_id from config by default, re-added client_id tests * minor fix of an error message * add Eq to bench_tps::cli::Congig * some minor error messages corrections

KirillLykov mentioned this pull request Feb 14, 2023

Node identity for bench #29929

Merged

KirillLykov requested a review from ilya-bobyr February 14, 2023 14:18

CriesofCarrots reviewed Feb 14, 2023

View reviewed changes

ilya-bobyr reviewed Feb 14, 2023

View reviewed changes

ilya-bobyr reviewed Feb 15, 2023

View reviewed changes

bench-tps/src/cli.rs Outdated Show resolved Hide resolved

bench-tps/src/main.rs Outdated Show resolved Hide resolved

ilya-bobyr reviewed Feb 15, 2023

View reviewed changes

ilya-bobyr requested changes Feb 17, 2023

View reviewed changes

ilya-bobyr mentioned this pull request Feb 17, 2023

Add Eq for Keypair #30381

Closed

KirillLykov force-pushed the refactor_parse_cli_bench_tps branch from b59c1a0 to 7a7105c Compare February 18, 2023 10:53

KirillLykov requested review from ilya-bobyr and CriesofCarrots February 19, 2023 12:54

KirillLykov force-pushed the refactor_parse_cli_bench_tps branch from e542f39 to 5542876 Compare February 23, 2023 09:42

dmakarov previously approved these changes Feb 23, 2023

View reviewed changes

CriesofCarrots previously approved these changes Feb 23, 2023

View reviewed changes

KirillLykov dismissed stale reviews from CriesofCarrots and dmakarov via fcad6aa February 23, 2023 17:16

KirillLykov force-pushed the refactor_parse_cli_bench_tps branch from 5542876 to fcad6aa Compare February 23, 2023 17:16

dmakarov previously approved these changes Feb 23, 2023

View reviewed changes

ilya-bobyr previously approved these changes Feb 23, 2023

View reviewed changes

KirillLykov dismissed stale reviews from ilya-bobyr and dmakarov via 1c35fe9 February 24, 2023 12:45

Kirill Lykov added 7 commits February 24, 2023 13:46

Improve cli args parsing in bench-tps

9a837f8

* Rename parse funciton * Get rid of panics, exit and println * Add basic unit tests

add allow dead_code and unused_imports for cli tests

11b1431

simplify code by using map_err instead of macro

c2219c7

simplify data_size parsing

4b2e49d

use tempfile crate to create temp keypair files

de737c8

fix bug with reading client_id from config by default, re-added clien…

c8c2b63

…t_id tests

minor fix of an error message

0219184

Kirill Lykov added 2 commits February 24, 2023 13:46

add Eq to bench_tps::cli::Congig

c34fc08

some minor error messages corrections

df748f2

KirillLykov force-pushed the refactor_parse_cli_bench_tps branch from 1c35fe9 to df748f2 Compare February 24, 2023 12:46

dmakarov approved these changes Feb 24, 2023

View reviewed changes

KirillLykov merged commit 2c37763 into solana-labs:master Feb 24, 2023

apfitzge mentioned this pull request Apr 27, 2023

Fix keypair multiplier check #31374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve cli args parsing in bench-tps #30307

Improve cli args parsing in bench-tps #30307

KirillLykov commented Feb 14, 2023 •

edited

Loading

KirillLykov commented Feb 14, 2023 •

edited

Loading

ilya-bobyr commented Feb 14, 2023

CriesofCarrots Feb 14, 2023

ilya-bobyr Feb 14, 2023

CriesofCarrots Feb 23, 2023

ilya-bobyr Feb 23, 2023

CriesofCarrots Feb 23, 2023

ilya-bobyr Feb 23, 2023

CriesofCarrots Feb 23, 2023

ilya-bobyr Feb 14, 2023

ilya-bobyr Feb 15, 2023

KirillLykov Feb 16, 2023

ilya-bobyr Feb 17, 2023

ilya-bobyr Feb 17, 2023

KirillLykov Feb 18, 2023

ilya-bobyr left a comment

ilya-bobyr Feb 17, 2023

ilya-bobyr Feb 17, 2023

ilya-bobyr Feb 17, 2023

KirillLykov Feb 18, 2023

CriesofCarrots Feb 23, 2023

ilya-bobyr left a comment

ilya-bobyr Feb 23, 2023

KirillLykov Feb 24, 2023

ilya-bobyr Feb 23, 2023

ilya-bobyr Feb 23, 2023

ilya-bobyr Feb 23, 2023

ilya-bobyr Feb 23, 2023

ilya-bobyr Feb 23, 2023

ilya-bobyr Feb 23, 2023

	solana_sdk::signature::{read_keypair_file, write_keypair_file, Keypair, Signer},
	solana_sdk::signature::{write_keypair_file, Signer},

	v.to_string().parse().map_err(\|_\| "can't parse lamports")?;
	v.to_string().parse().map_err(\|_\| "can't parse target-lamports-per-signature")?;

	.map_err(\|_\| "can't parse target slots per epoch")?;
	.map_err(\|_\| "can't parse target-slots-per-epoch")?;

	.map_err(\|_\| "Can't parse conflict groups")?;
	.map_err(\|_\| "Can't parse num-conflict-groups")?;

	solana_net_utils::parse_host(addr).map_err(\|_\| "Failed to parse bind_address")?;
	solana_net_utils::parse_host(addr).map_err(\|_\| "Failed to parse bind-address")?;

Improve cli args parsing in bench-tps #30307

Improve cli args parsing in bench-tps #30307

Conversation

KirillLykov commented Feb 14, 2023 • edited Loading

Problem

Summary of Changes

KirillLykov commented Feb 14, 2023 • edited Loading

ilya-bobyr commented Feb 14, 2023

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

ilya-bobyr 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

ilya-bobyr 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

KirillLykov commented Feb 14, 2023 •

edited

Loading

KirillLykov commented Feb 14, 2023 •

edited

Loading