Ts/extractor/migrate data extractor to rust

[SouchonTheo]

Ports ci/data_extractor to Rust as utils/tfhe-data-extractor, plus the benchmark_spec work it needs.

benchmark_spec gains FromStr down the whole bench tree, so an id parses back through the grammar it renders with, and now owns the stored result name {bench_id}_{statistic}(_{variant})? that tfhe-benchmark-parser composes. What lands in the results database is unchanged.

The extractor resolves a regression profile to bench paths, turns them into anchored SQL LIKE patterns, and renders the integer, ERC7984 and KV-store tables as Markdown, CSV or SVG. Regression JSON, backends comparison, the wasm layer and the shortint and core_crypto tables are not ported and fail explicitly, so the Python tool stays for now.

AI was used along the way to keep the python context and correct the code written

Here is an example of the rust version

---

This change is 

[github-actions]

✅ Backward-compat snapshot: everything looks good! No backward-compatibility issues detected.

[tmontaigu]

utils/tfhe-data-extractor/src/format/integer.rs line 74 at r1 (raw file):

    ),
    (
        "Leading / Trailing zeros/ones",

There is also no leading zeros/ones, ilog2 for scalars, just like there is no negation as they are unary operation

[SouchonTheo]

@SouchonTheo made 1 comment.
Reviewable status: 0 of 44 files reviewed, 1 unresolved discussion (waiting on IceTDrinker, nsarlin-zama, soonum, and tmontaigu).

---

utils/tfhe-data-extractor/src/format/integer.rs line 74 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

There is also no leading zeros/ones, ilog2 for scalars, just like there is no negation as they are unary operation

oh wow good catch
in fine the line was removed because of the logic behind the code
(remove empty lines) (python was doing the same stuff)
There is also select to remove right ?

I did not have the chance to try scalar for now but it should work now !

[tmontaigu]

utils/tfhe-data-extractor/src/format/integer.rs line 74 at r1 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

oh wow good catch
in fine the line was removed because of the logic behind the code
(remove empty lines) (python was doing the same stuff)
There is also select to remove right ?

I did not have the chance to try scalar for now but it should work now !

yes there is select(FheBool, Clear, Fhe), elect(FheBool, Fhe, Clear), select(FheBool, Clear, Clear),

[soonum]

Awesome job thanks! Some details needs attention though.

@soonum partially reviewed 43 files and made 10 comments.
Reviewable status: 42 of 44 files reviewed, 10 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and SouchonTheo).

---

utils/benchmark_spec/src/lib.rs line 240 at r1 (raw file):

impl BenchmarkSpec {
    /// The benchmarked operation, as a path (`tfhe::hlapi::ops::add`).

This method doesn't seem to match the comment description.

---

utils/benchmark_spec/src/lib.rs line 255 at r1 (raw file):

    }

    /// Whether the second operand is a plaintext (a `scalar` benchmark).

Can it be scalar only or is defaulting to ciphertext?

---

utils/benchmark_spec/src/measured.rs line 1 at r1 (raw file):

//! The name a benchmark result is stored under:

This sentence feels off. Maybe it needs a The name *of* a benchmark...

---

utils/benchmark_spec/src/parse.rs line 25 at r1 (raw file):

        let mut it = trailing.split("::").filter(|t| !t.is_empty()).peekable();

        let backend = match it.peek().copied() {

Why using a peek().copied() call? Can't we just use the reference of it?

---

utils/tfhe-data-extractor/config.example.toml line 4 at r1 (raw file):

# `--config-file config.toml`.
#
# Environment variables OVERRIDE any value below (matching the Python tool):

Remove the (matching the Python tool) part since we'll drop the Python tool in the end.

---

utils/tfhe-data-extractor/src/main.rs line 441 at r1 (raw file):

            (Layer::Integer, BenchSubset::All) => vec![
                (
                    "-ciphertext".to_string(),

Can we review together to ensure suffixing matches the public documentation assets naming (or close enough)?
Currently I have to rename file produced to the one stored in the gitbook directory.

---

utils/tfhe-data-extractor/src/profile.rs line 1 at r1 (raw file):

//! Regression profiles (`ci/regression.toml`): the list of benchmarks a report

Have you tested to run a regression data extraction?

---

utils/tfhe-data-extractor/src/format/kv_store.rs line 52 at r1 (raw file):

    }

    let conflicts = cells.conflicts();

I don't understand the logic behind handling the "conflicts" in the block afterward. Can you explain it?

---

utils/tfhe-data-extractor/src/format/render/svg.rs line 1 at r1 (raw file):

//! SVG table, as published in the documentation: fixed geometry, a black header

No crates dedicated to SVG rendering? We are rendering the cells sizes and position by hand here which is brittle and not really scalable.
Back then I didn't find any Python module that suited my needs.

[soonum]

utils/tfhe-data-extractor/src/format/mod.rs line 70 at r1 (raw file):

        if !self.conflicts.is_empty() {
            eprintln!(
                "warning: {} cell(s) matched by several benchmarks, first kept. \

\n at the end of the line no?

[github-actions]

✅ Forward compatibility matrix unchanged: it still matches the committed baseline.

Matrix

Which released versions can load data produced by this branch. A ❌ is not a failure by itself: it only means forward compatibility is not (or no longer) provided for that type. What matters is whether it was reviewed.

TYPE	1.5.5	1.6.3	1.7.0
CompactPublicKey	❌	✅	✅
CompactPkeCrs	✅	✅	✅
ProvenCompactCiphertextList	❌	✅	✅

[SouchonTheo]

@SouchonTheo made 11 comments.
Reviewable status: 34 of 44 files reviewed, 11 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, soonum, and tmontaigu).

---

utils/benchmark_spec/src/lib.rs line 240 at r1 (raw file):

Previously, soonum (David Testé) wrote…

This method doesn't seem to match the comment description.

yes it is
like bench crate is the first part of the full spec you have the example
it describe the operation in fine like the add of the hlapi ops
but maybe it's not making sense

do you have something else in head ?

---

utils/benchmark_spec/src/lib.rs line 255 at r1 (raw file):

Previously, soonum (David Testé) wrote…

Can it be scalar only or is defaulting to ciphertext?

it is either ciphertext or plaintext

---

utils/benchmark_spec/src/measured.rs line 1 at r1 (raw file):

Previously, soonum (David Testé) wrote…

This sentence feels off. Maybe it needs a The name *of* a benchmark...

replaced by
Composes and parses the name of a stored benchmark result

---

utils/benchmark_spec/src/parse.rs line 25 at r1 (raw file):

Previously, soonum (David Testé) wrote…

Why using a peek().copied() call? Can't we just use the reference of it?

I had issue to make it work correctly
after some attempt I asked AI for cleaner solution it came up with

  let backend = it
      .next_if(|t| matches!(*t, "cuda" | "hpu"))
      .map_or(Backend::Cpu, |t| match t {
          "cuda" => Backend::Cuda,
          _ => Backend::Hpu,
      });

  let operand_type = match it.next_if_eq(&"scalar") {
      Some(_) => OperandType::PlainText,
      None => OperandType::CipherText,
  };

which is directly related to the peakable function

honestly I kinda like that wdyt ?

---

utils/tfhe-data-extractor/config.example.toml line 4 at r1 (raw file):

Previously, soonum (David Testé) wrote…

Remove the (matching the Python tool) part since we'll drop the Python tool in the end.

done

---

utils/tfhe-data-extractor/src/main.rs line 441 at r1 (raw file):

Previously, soonum (David Testé) wrote…

Can we review together to ensure suffixing matches the public documentation assets naming (or close enough)?
Currently I have to rename file produced to the one stored in the gitbook directory.

oh yes of course
we can make it as we don't need to rename afterward (hope it's possible)

---

utils/tfhe-data-extractor/src/profile.rs line 1 at r1 (raw file):

Previously, soonum (David Testé) wrote…

Have you tested to run a regression data extraction?

yes we have a test for it
and also the example in pr came from directly from the code/db of prod

---

utils/tfhe-data-extractor/src/format/integer.rs line 74 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

yes there is select(FheBool, Clear, Fhe), elect(FheBool, Fhe, Clear), select(FheBool, Clear, Clear),

ok I will add it back select but it's never benched for scalar

---

utils/tfhe-data-extractor/src/format/kv_store.rs line 52 at r1 (raw file):

Previously, soonum (David Testé) wrote…

I don't understand the logic behind handling the "conflicts" in the block afterward. Can you explain it?

you are right some work is duplicated
there is no real reason to have this like that
I will update the code and also in other file to make it cleaner

I even find a way to simplify even more
it change all the format file and also a little bit the main with a new type Measured which create the missing glue I needed

---

utils/tfhe-data-extractor/src/format/mod.rs line 70 at r1 (raw file):

Previously, soonum (David Testé) wrote…

\n at the end of the line no?

not really as we make the new line ourselve

---

utils/tfhe-data-extractor/src/format/render/svg.rs line 1 at r1 (raw file):

Previously, soonum (David Testé) wrote…

No crates dedicated to SVG rendering? We are rendering the cells sizes and position by hand here which is brittle and not really scalable.
Back then I didn't find any Python module that suited my needs.

I think we will go without any crates for now

[tmontaigu]

@tmontaigu reviewed 21 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 39 of 44 files reviewed, 12 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, soonum, and SouchonTheo).

---

utils/benchmark_spec/src/measured.rs line 36 at r4 (raw file):

/// Takes the bench id as a string, not a [`BenchmarkSpec`]: results are written
/// for every benchmark, including those whose id predates the spec grammar.
pub fn measured_name(bench_id: &str, statistic: Statistic, variant: Option<&str>) -> String {

Is this used only in this file ?
If so then I thik this function does not need to exist:

I would put its code in the fmt::Display impl for MeasureId as write!(f, {bench_id}{}, ...), if ...
and in the test display_matches_the_writer simply compare to a hardcoded expected string, because at the moment the test it kind of testing that the same function call twice retuns the same thing

---

utils/benchmark_spec/src/measured.rs line 75 at r4 (raw file):

            spec: bench_id.parse()?,
            statistic,
            variant: match rest.strip_prefix('_') {

seems like we could use and_then https://doc.rust-lang.org/std/option/enum.Option.html#method.and_then

rest
    .string_prefix('_')
    .and_then(|variant| if variant.is_empty() { None } else { Some(variant.to_string))

[nsarlin-zama]

looks solid, some small comments.
I'm not a db expert however :)

@nsarlin-zama partially reviewed 44 files and made 8 comments.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).

---

utils/benchmark_spec/src/lib.rs line 240 at r1 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

yes it is
like bench crate is the first part of the full spec you have the example
it describe the operation in fine like the add of the hlapi ops
but maybe it's not making sense

do you have something else in head ?

I think what's confusing is that since the name is bench_crate and it returns a BenchCrate, we imagine that it will only return the "crate" part of the path (like "tfhe").

But I understand that recursively, it holds the full path. Maybe naming could be modified to fix this (bench_path(&self) -> BenchPath ? or bench_path(&self) -> BenchCrate if modifying the type does not make sense at the global scale).

Looking at how it is used below, I think renaming the type to BenchPath maybe makes a bit more sense, if it's not too complex

---

utils/benchmark_spec/src/parse.rs line 25 at r1 (raw file):

Previously, SouchonTheo (Theo Souchon) wrote…

I had issue to make it work correctly
after some attempt I asked AI for cleaner solution it came up with

  let backend = it
      .next_if(|t| matches!(*t, "cuda" | "hpu"))
      .map_or(Backend::Cpu, |t| match t {
          "cuda" => Backend::Cuda,
          _ => Backend::Hpu,
      });

  let operand_type = match it.next_if_eq(&"scalar") {
      Some(_) => OperandType::PlainText,
      None => OperandType::CipherText,
  };

which is directly related to the peakable function

honestly I kinda like that wdyt ?

personally I don't mind the "peek" but the "next_if" version is maybe a bit easier to read

---

utils/tfhe-data-extractor/config.example.toml line 1 at r3 (raw file):

# Copy this file to `config.toml` and fill in the credentials, then pass it with

If it makes sense to execute this locally, maybe a short README.md explaining how could be useful? Or just a global doc for this and the benchmark parser explaining how to use the full process?

---

utils/benchmark_spec/src/parse.rs line 150 at r3 (raw file):

        }
    }

maybe you could add a bit more "invalid" tests, like something starting with "::", or ending with it, or with "::::" somewhere in the middle, or that ends too soon, or with too many items,... or whatever you can think of !

---

utils/tfhe-data-extractor/src/db.rs line 73 at r3 (raw file):

            .password(password)
            .database(dbname)
            // RDS enforces TLS; `Require` encrypts without CA verification.

in ci/prod what is the config? If the db is not on the same machine are we fine with that?

---

utils/tfhe-data-extractor/src/main.rs line 119 at r3 (raw file):

    /// Hardware reference used to perform benchmark.
    #[arg(short = 'w', long, default_value = "hpc8a.96xlarge")]

maybe this should not have a default value. If the caller use the default value, we might not think about changing this if someday we migrate to another machine

---

utils/tfhe-data-extractor/src/format/erc7984.rs line 17 at r3 (raw file):

/// Rows in publication order. HPU publishes a different set (`hpu_optim`,
/// `hpu_simd`) that is not covered here.

From TransferFlavor, it looks like "Safe" is not covered, is it expected?