composer: add initial version #75

jxs · 2022-11-18T21:10:45Z

This is an initial version of the composer tool. I am submitting this as a draft PR so we can broaden the discussion regarding the assumptions I did.
I followed the idea of using csv files with sqlite from @MarcoPolo which turned great cause it allows us to model the data and create all the combinations in a much simpler way using sql.

But due to the difficulty of modeling all the test and build parameters into csv files, including the different natures of go and rust test plans which use different builders, and after speaking with @laurentsenta and @MarcoPolo it occurred to me that if we simplify on the builder to be docker:generic and, pass the version as we currently do via [groups.build_config.build_args]
and pass all the test parameters (transport, muxer, protocol) when launching the container the work becomes a lot easier for the composer and we could also combine it with testground/testground#1522.
Does testground currently support passing env vars when launching containers @laurentsenta?

While on the Go side we'd had to change the test plans to start using docker:generic and change the way the parameters are passed, the changes required on the Rust side we'd just to rename the binary files to match the VERSION passed on the composition.toml file.

Wdyt of using just docker:generic to simplify the process @laurentsenta @mxinden?
a run of composer with these csv files produces the following composition.toml

Thanks.

MarcoPolo · 2022-11-18T21:14:54Z

does testground support passing env vars when launching containers @laurentsenta?

Pretty sure it must because this is how the testground SDK gets all the information.

p-shahi · 2022-11-18T21:20:48Z

Drive by: not entirely certain as I'm not following closely but might need testground/testground#1505 to pair with testground/testground#1522

p-shahi · 2022-11-18T21:29:03Z

composer/muxers.csv

@@ -0,0 +1,11 @@
+id,muxer


Naive question, will there be an additional csv for security protocol TLS/Noise, or is that being handled differently?

will be another file, i.e. sec.csv:

id,protocol rust-master,noise rust-master, tls rust-0.49.0,noise rust-0.49.0,tls

that is then queried along the other ones.
The process of adding implementations can be improved later, either by creating a cli tool that does it or, getting another primary format, that is then parsed by the composer and converted to sql

thomaseizinger

Thanks!

I added some thoughts :)

thomaseizinger · 2022-11-20T23:40:04Z

composer/package.json

+{
+  "name": "composer",
+  "version": "0.0.1",
+  "description": "",
+  "main": "index.ts",
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"
+  },
+  "author": "",
+  "license": "ISC",
+  "devDependencies": {
+    "@types/node": "^18.8.4",
+    "@types/yargs": "^17.0.13",
+    "ts-node": "^10.9.1",
+    "typescript": "^4.8.4"
+  },
+  "dependencies": {
+    "@iarna/toml": "^2.2.5",
+    "@types/iarna__toml": "^2.0.2",
+    "sqlite": "^4.1.2",
+    "sqlite3": "^5.1.2",
+    "yargs": "^17.6.2"
+  }
+}


Has DENO been considered as an alternative? It would allow us to put everything in a single file without having to install dependencies etc.

Thanks for bringing this, yeah I considered but as it wasn't present by default on ubuntu-latest and we weren't using container images on the workflow I didn't want to introduce it. But if we can I am happy to use it if everyone agrees.

Installing it seems to be easy enough: https://github.com/denoland/setup-deno

I think this would be a good usecase for it, should make this script a lot more maintainable.

thomaseizinger · 2022-11-20T23:41:34Z

composer/index.ts

+import sqlite3 from "sqlite3";
+import { open } from "sqlite";


What is the difference between these two packages?

sqlite is a wrapper library that uses sqlite3 as the driver.

thomaseizinger · 2022-11-20T23:42:42Z

composer/index.ts

+    // Call sqlite to process the csv resource files and generate a database.
+    // We call the sqlite process instead of doing it here cause
+    // the dot commands are interpreted by the sqlite cli tool not sqlite itself,
+    // and it is a lot faster parsing the csv's.


Our CSVs are <10 lines long. What am I meant to take away from this comment?

that we can't call sqlite dot commands via the database driver, therefore we do it via execFile therefore instead of parsing the csv file with js, we do it with sqlite instead cause it's faster.

Our CSVs are <10 lines long.

but they are expected to grow I assume, do you suggest importing, parsing and inserting in typescript land instead?

Our CSVs are <10 lines long.

but they are expected to grow I assume

I am guessing they would have to grow into the thousands for performance to become a problem and thus a driving factor for the design. I hope that we won't have CSV files that are 1000s of lines long!

do you suggest importing, parsing and inserting in typescript land instead?

I think we should do whatever results in the better design. If we want to have a test for this, I think it will likely be easier if we don't need to set up dummy files for the tests but can pass data in as variables. That would suggest that we use SQL to insert the data which in turn means parsing the files in TS.

thomaseizinger · 2022-11-20T23:43:57Z

composer/index.ts

+    const db = await open({
+        filename: DB,
+        driver: sqlite3.Database,
+    });
+
+    // Generate the testing combinations by SELECT'ing from both transports
+    // and muxers tables the distinct combinations where the transport and the muxer
+    // of the different libp2p implementations match.
+    const queryResults =
+        await db.all(`SELECT DISTINCT a.id as id1, b.id as id2, a.transport, ma.muxer
+                     FROM transports a, transports b, muxers ma, muxers mb
+                     WHERE a.id != b.id
+                     AND a.transport == b.transport
+                     AND a.id == ma.id
+                     AND b.id == mb.id
+                     AND ma.muxer == mb.muxer;`);
+    await db.close();
+
+    let global = new Global(
+        "multidimensional-testing",
+        "multidimensional",
+        argv.total_instances
+    );
+    let composition = new Composition(global, []);
+
+    for (let row of queryResults) {
+        // Instance count is hardcoded to 1 for now.
+        let instance = new Instance(1);
+
+        let build_args1 = new BuildArgs(
+            row.transport,
+            row.muxer,
+            row.id1,
+            argv.gitRef,
+            argv.gitTarget
+        );
+        let build_config1 = new BuildConfig(build_args1);
+        let group1 = new Group(row.id1, BUILDER, instance, build_config1);
+
+        let build_args2 = new BuildArgs(
+            row.transport,
+            row.muxer,
+            row.id2,
+            argv.gitRef,
+            argv.gitTarget
+        );
+        let build_config2 = new BuildConfig(build_args2);
+        let group2 = new Group(row.id2, BUILDER, instance, build_config2);
+
+        let run = new Run(
+            `${row.id1} x ${row.id2} x ${row.transport} x ${row.muxer}`,
+            [group1, group2]
+        );
+
+        composition.runs.push(run);
+    }


It would be nice to have a test for this snippet of code.

So we can make sure it works.

To make review easier: I don't have to run this locally to see the output.

Yeah agree, and I am planning to do it after closing the spec.

thomaseizinger · 2022-11-20T23:46:20Z

composer/transports.csv

+rust-master,tcp
+rust-master,quic
+rust-master,tcp
+rust-0.49.0,tcp
+rust-0.48.0,tcp
+rust-0.47.0,tcp
+go-0.23.4,tcp
+go-0.23.4,quic


This is going to get really repetitive (and error prone, see lines 2 and 4 are duplicates) as we release new versions. If we already build a tool that generates the combinations for us, wouldn't it make more sense to track a semver specifier that marks, which versions support which feature?

What we would track is:

language quic yamux mplex tcp

rust > 0.50.0 > 0.20.0¹ ... ...

go > 0.24.3 ... ... ...

A null entry would mean unsupported.

If we ever decide to remove a feature, we can use a semver range: > 0.50.0 < 0.57.0.

Given this table, we only need one other list for each language that specifies the versions we want to test, e.g. for Rust, that could say:

0.45.0

0.46.0

...

I am not sure if sqlite is still the best tool for this. We might be able to use it to generate the combinations initially but throw out the ones that don't match the semver range. There might also be a module that can do semver parsing in sqlite, I haven't checked.

In any case, I think this would be a lot more maintainable going forward.

Thoughts?

Footnotes

Just making this up. ↩

this links with @p-shahi's question on #75 (comment) and your comment on #75 (comment) I think.

Yeah this seems to me an elegant solution to solve the increasing number of dimensions into csv files and I don't think it is incompatible with using sqlite. I.e we could define the format suggested above with semver's and then transform it into the csv "tables" and insert them into sqlite tables for then query them.

But I'd leave this to a subsequent phase, as a improvement to the Extract part of the ETL. Wdyt?

I like this as well! agree with @jxs this should be a next step rather than a requirement for the MVP.

I am okay with doing it in a follow-up if we think that the overall design holds.

Your reference to ETL is interesting. I hadn't considered that we can use the suggested table to then produce the current ones. I would have seen that as an unnecessary intermediate step but perhaps it is easier to implement if we do the generation in multiple steps, thus forming a pipeline.

laurentsenta · 2022-11-21T10:08:28Z

composer/index.ts

+    .catch((err) => {
+        console.error(err);
+        process.exit(1);
+    });


(creating here just to get a thread)

pass all the test parameters (transport, muxer, protocol) when launching the container the work becomes a lot easier for the composer and we could also combine it with testground/testground#1522.
Does testground currently support passing env vars when launching containers @laurentsenta?

Yes, the idiomatic solution is to use the run parameters,
there is an example of this in the ping test:

test-plans/ping/go/main.go

Lines 69 to 71 in abce1ee

secureChannel = runenv.StringParam("secure_channel")

maxLatencyMs = runenv.IntParam("max_latency_ms")

iterations = runenv.IntParam("iterations")

You'd pass the parameters in the runs directly or in the runs.group field:

test-plans/composer/demo/composition-example.toml

Lines 79 to 80 in 3fccfb2

[runs.test_params]

iterations = "5"

laurentsenta · 2022-11-21T10:57:07Z

composer/index.ts

+    .then(() => process.exit(0))
+    .catch((err) => {
+        console.error(err);
+        process.exit(1);


(another thread)

Wdyt of using just docker:generic to simplify the process @laurentsenta @mxinden?

If you want to tackle this question as part of this PR:

Initially, we planned on:

Implementing the test using testground builders + runners,

Rely on docker:generic and docker_extensions to implement missing features,

As we converge to a reliable solution, make the features Testground natives (add a docker:rust builder for example, and other simplifications).

With this approach, I would NOT recommend moving to docker:generic, but instead moving features into testground.

It seems we're now moving towards a different design:

Implementing the tests using [external] builders + testground's runners.

related: Allow plans to point to an existing docker image testground/testground#1522

With this approach, I would recommend investing in external builders rather than rewriting what we have.

- the first should be runtime parameters whereas version should be a build parameters - separate Go and Rust git ref and target

p-shahi · 2022-12-05T20:32:35Z

@jxs What's the remaining work to get the composer in (& promote to ready for review)? #88 is not using this yet right?

MarcoPolo · 2022-12-05T20:54:50Z

@p-shahi i think we'll continue this work in #85. And we'll extend #88 to support this test matrix (#88 I don't think #88 is using this yet).

p-shahi · 2022-12-10T23:56:11Z

closing in favor of #85

composer: add initial version

ad11458

p-shahi reviewed Nov 18, 2022

View reviewed changes

p-shahi mentioned this pull request Nov 18, 2022

[tracking issue] Interop test-plans for all existing/developing libp2p transports #61

Open

28 tasks

jxs marked this pull request as draft November 19, 2022 02:45

thomaseizinger reviewed Nov 21, 2022

View reviewed changes

laurentsenta reviewed Nov 21, 2022

View reviewed changes

composer: separate transport and muxers from version,

0c4ce07

- the first should be runtime parameters whereas version should be a build parameters - separate Go and Rust git ref and target

tinytb mentioned this pull request Nov 28, 2022

Create a "simple" interop dashboard #55

Closed

13 tasks

p-shahi closed this Dec 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

composer: add initial version #75

composer: add initial version #75

jxs commented Nov 18, 2022 •

edited

Loading

MarcoPolo commented Nov 18, 2022

p-shahi commented Nov 18, 2022 •

edited

Loading

p-shahi Nov 18, 2022

jxs Nov 18, 2022

thomaseizinger left a comment

thomaseizinger Nov 20, 2022

jxs Nov 21, 2022

thomaseizinger Nov 23, 2022

thomaseizinger Nov 20, 2022

jxs Nov 21, 2022

thomaseizinger Nov 20, 2022

jxs Nov 21, 2022 •

edited

Loading

thomaseizinger Nov 23, 2022

thomaseizinger Nov 20, 2022

jxs Nov 21, 2022

thomaseizinger Nov 20, 2022

jxs Nov 21, 2022

MarcoPolo Nov 21, 2022

thomaseizinger Nov 23, 2022

laurentsenta Nov 21, 2022

laurentsenta Nov 21, 2022

p-shahi commented Dec 5, 2022

MarcoPolo commented Dec 5, 2022

p-shahi commented Dec 10, 2022

		import sqlite3 from "sqlite3";
		import { open } from "sqlite";

	secureChannel = runenv.StringParam("secure_channel")
	maxLatencyMs = runenv.IntParam("max_latency_ms")
	iterations = runenv.IntParam("iterations")

composer: add initial version #75

composer: add initial version #75

Conversation

jxs commented Nov 18, 2022 • edited Loading

MarcoPolo commented Nov 18, 2022

p-shahi commented Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger 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

jxs Nov 21, 2022 • 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

Footnotes

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

p-shahi commented Dec 5, 2022

MarcoPolo commented Dec 5, 2022

p-shahi commented Dec 10, 2022

jxs commented Nov 18, 2022 •

edited

Loading

p-shahi commented Nov 18, 2022 •

edited

Loading

jxs Nov 21, 2022 •

edited

Loading