Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Tests Script for preparing ACVP python script Integration #234

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

potsrevennil
Copy link
Contributor

@potsrevennil potsrevennil commented Oct 14, 2024

Refactor tests script mainly for preparation to easily integrate acvp into tests:

  • Split the original tests script into three files: the main scripts/tests script now acts as a Click-based CLI interface.
  • Moved core testing logic to scripts/lib/test.py, providing APIs that the Click commands call for main testing functionality.
  • Added scripts/lib/util.py for common type classes and utility functions.
  • Rename the State class as mentioned in https://github.com/pq-code-package/mlkem-c-aarch64/pull/347#discussion_r1830477719)
  • Make tests func/kat/nistkat/all interface consistent:
    • --opt flags become choice of ALL | OPT | NO_OPT (case insensitive)

@potsrevennil potsrevennil added the benchmark this PR should be benchmarked in CI label Oct 14, 2024
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch 7 times, most recently from 7005190 to aaedf55 Compare October 14, 2024 17:31
@mkannwischer mkannwischer added benchmark this PR should be benchmarked in CI and removed benchmark this PR should be benchmarked in CI labels Oct 25, 2024
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch 4 times, most recently from 4379e03 to 70495b1 Compare November 1, 2024 10:19
@potsrevennil potsrevennil changed the title Refactor build system to be able to build opt/non-opt binaries at the same time Fix tests all not working if tests is not in PATH and add suffix to binaries to allow building opt/non-opt binaries at the same time Nov 4, 2024
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch from 70495b1 to 45518b6 Compare November 4, 2024 02:51
@potsrevennil potsrevennil marked this pull request as ready for review November 4, 2024 02:54
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch 4 times, most recently from 631ff9e to 3623250 Compare November 6, 2024 05:45
@potsrevennil potsrevennil changed the title Fix tests all not working if tests is not in PATH and add suffix to binaries to allow building opt/non-opt binaries at the same time Adjust buildsystem to allow building opt/non-opt binaries at the same time Nov 6, 2024
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch 8 times, most recently from 821b530 to dbbdef2 Compare November 7, 2024 05:40
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch 2 times, most recently from 482679c to bdbf7fd Compare November 8, 2024 07:23
@potsrevennil potsrevennil added benchmark this PR should be benchmarked in CI and removed benchmark this PR should be benchmarked in CI labels Nov 8, 2024
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch from bdbf7fd to 7eadeb2 Compare November 8, 2024 07:39
@potsrevennil potsrevennil added benchmark this PR should be benchmarked in CI and removed benchmark this PR should be benchmarked in CI labels Nov 8, 2024
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch from 7eadeb2 to 453f5a4 Compare November 8, 2024 08:20
@potsrevennil potsrevennil added benchmark this PR should be benchmarked in CI and removed benchmark this PR should be benchmarked in CI labels Nov 8, 2024
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch from 453f5a4 to 659cd88 Compare November 8, 2024 08:26
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @potsrevennil for the refactoring - this looks cleaner to me.
I still need to go through the changes in detail, but maybe you can already have a look at this:
$ tests bench --opt all does not work the way I would expect it - it only runs the opt tests.

.github/actions/functest/action.yml Outdated Show resolved Hide resolved
.github/workflows/ci_ec2_any.yml Outdated Show resolved Hide resolved
@mkannwischer mkannwischer added benchmark this PR should be benchmarked in CI and removed benchmark this PR should be benchmarked in CI labels Nov 8, 2024
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch 3 times, most recently from 3a652ce to d334dbc Compare November 8, 2024 09:56
Tests are bounded by IO, so running tests asynchronously doesn't add
value, instead add more complexity only. Therefore avoiding running test
binaries with asyncio for simplicity

Signed-off-by: Thing-han, Lim <[email protected]>
Make logging format look nicer by utilizing the logger name, decorated by
test type, scheme, compile mode and implementation

Signed-off-by: Thing-han, Lim <[email protected]>
`State` class naming was a mistake, renaming it to Options

And refactor the member functions of State into member functions of
`Base`, `Test_Implementations`, `Tests`

`Base`: the lowest level class for handling compilation and binary
execution
`Test_Implementations`: handle running one or multiple implementations
`Tests`: provide top level functions for click cli commands

Signed-off-by: Thing-han, Lim <[email protected]>
@potsrevennil potsrevennil force-pushed the refactor-build-system-opt branch from d334dbc to e2aa705 Compare November 11, 2024 07:58
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @potsrevennil.

This still doesn't do what I'd expect it to do: nevermind - I was running an old version - let me try again.

$ tests bench --opt all --cycles PERF
None
INFO  > Benchmark       Compile     (native,    opt) make CROSS_PREFIX= bench CYCLES=PERF OPT=1 AUTO=1
INFO  > Benchmark       ML-KEM-512  (native,    opt)    keypair cycles = 24255
    encaps cycles = 30724
    decaps cycles = 39536

           percentile      1     10     20     30     40     50     60     70     80     90     99
   keypair percentiles:  23877  23975  24052  24114  24162  24255  24338  24450  24615  24919  26850
    encaps percentiles:  30305  30454  30519  30590  30652  30724  30815  30929  31134  31487  35613
    decaps percentiles:  39076  39228  39304  39393  39456  39536  39640  39727  39907  40208  42464

INFO  > Benchmark       ML-KEM-768  (native,    opt)    keypair cycles = 43647
    encaps cycles = 47745
    decaps cycles = 60480

           percentile      1     10     20     30     40     50     60     70     80     90     99
   keypair percentiles:  41991  42248  42475  42824  43240  43647  44009  44328  44817  45580  48292
    encaps percentiles:  45859  46158  46361  46649  47284  47745  48064  48347  48944  49807  52009
    decaps percentiles:  58633  58936  59164  59469  60010  60480  60869  61219  61821  62552  64655

INFO  > Benchmark       ML-KEM-1024 (native,    opt)    keypair cycles = 62950
    encaps cycles = 69488
    decaps cycles = 87193

           percentile      1     10     20     30     40     50     60     70     80     90     99
   keypair percentiles:  60969  61673  62105  62409  62658  62950  63238  63565  64200  64894  67050
    encaps percentiles:  67655  68272  68630  68972  69238  69488  69886  70193  70607  71357  73030
    decaps percentiles:  85259  85922  86317  86630  86906  87193  87540  87929  88307  89070  90809

@mkannwischer mkannwischer added benchmark this PR should be benchmarked in CI and removed benchmark this PR should be benchmarked in CI labels Nov 11, 2024
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @potsrevennil - this looks good though.
Hopefully the last rework of the entire test script.

One downside is that we now no longer check the namespacing for the no-opt implementation in the quickcheck CI. Let's do that in a follow-up. I opened an issue #372

@mkannwischer mkannwischer merged commit 9c75556 into main Nov 11, 2024
92 checks passed
@mkannwischer mkannwischer deleted the refactor-build-system-opt branch November 11, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark this PR should be benchmarked in CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants