Fix tests bench --opt=all and tests all --no-acvp #398
Conversation
potsrevennil
force-pushed
the
fix-tests
branch
2 times, most recently
from
ccefa52 to
754ef24
Compare
potsrevennil
force-pushed
the
fix-tests
branch
from
754ef24 to
9648698
Compare
There was a problem hiding this comment.
Thank you @potsrevennil. As discussed offline, please don't mix bugfixes and refactoring anymore in the future.
For the review of the bugfix, could you please describe in the commit message what the bug was in the first place (What did you do? What was the expected behaviour? What was the actual behaviour?) and brieflt how it is fixed?
potsrevennil
force-pushed
the
fix-tests
branch
from
9648698 to
5b28a2f
Compare
potsrevennil
force-pushed
the
fix-tests
branch
from
5b28a2f to
3a3f4dd
Compare
|
@hanno-becker Done |
|
#407 cherry-picks first commit. |
potsrevennil
force-pushed
the
fix-tests
branch
3 times, most recently
from
95b89db to
4535d9f
Compare
|
I tried to make the git diff looked friendlier, hope that it'd be clear that this was just a simple rewrite, which is just merging the conditional |
|
@potsrevennil I seem to remember there a different diff options locally at least... anyone in particular that will make it more obvious what is happening? |
hanno-becker
force-pushed
the
fix-tests
branch
from
4535d9f to
abfaf2d
Compare
scripts/lib/mlkem_test.py
Outdated
| @@ -307,32 +319,27 @@ def expect(scheme: SCHEME, actual: str) -> tuple[bool, str]: | |||
| f"Failed, expecting {expect}, but getting {actual}" if fail else "", | |||
| ) | |||
|
|
|||
| return self._func.run_schemes( | |||
| return self._f(TEST_TYPES.MLKEM).test( | |||
There was a problem hiding this comment.
From the commit message I was expecting to see pairs of compile+run being replaced by test, but here we only call run_schemes before, while the new call to test() would additionally compile. Is that deliberate? Do we end up compiling more often than we did before?
There was a problem hiding this comment.
originally, the pairs of compile+run is in the local function _func in func
so the self._func is actually a replacement of the local function _func in self.func
so before the function calls was like
func -> local _func -> self._func.compile+ self._run_func -> self._func.run_schemes
all -> self._func.compile + self._run_func -> self._func.run_schemes
which is pretty hard to understand
it now became
func -> self._func -> self.__func.test -> self.__func.compile + self.__func.run_schemes
all -> self._func -> self.__func.test -> self.__func.compile + self.__func.run_schemes
scripts/lib/mlkem_test.py
Outdated
| self.compile_mode = copts.compile_mode() | ||
| self.compile = opts.compile | ||
| self.run = opts.run | ||
|
|
||
| def _run_func(self, opt: bool): | ||
| def _func(self, opt: bool, compile: bool, run: bool) -> TypedDict: |
There was a problem hiding this comment.
Why is this indirection needed? Can we not inline all of this into self.func?
There was a problem hiding this comment.
We could, but in the functions like self.func, there's logger configuration,
which means if I called self.func instead in all, then the logger would be configured multiple times.
and also in self.func if --opt=all it'd compile/run both the implementations, but in all we need to first compile/run one implementation then do the other due to the current build system restriction, therefore if without the indirection, there might required more changes in all
As we have discussed about build system change in the future (separate opt/non-opt build artifacts), once it's done, we could thus remove this annoying indirection by the way.
scripts/lib/mlkem_test.py
Outdated
| @@ -526,29 +525,32 @@ def init_results() -> TypedDict: | |||
|
|
|||
| return fail | |||
|
|
|||
| def _acvp(self, opt: bool, compmile: bool, run: bool, acvp_dir: str) -> bool: | |||
There was a problem hiding this comment.
Why is this handled differently from say _func? In _func, you just call self._f(...).test(), which bundles compile and run -- while here you still call both separately. Note also that L530 and L532 do not operate on the same test object -- they call _f twice.
There was a problem hiding this comment.
because the way we run acvp is very different from the other tests, the other tests only run once for each parameter set, but acvp test is run multiple times for each parameter set with different test vectors.
it's too hard and not necessary to reuse the same function test for now.
There was a problem hiding this comment.
I don't think this change makes the test script easier to understand than before. On the contrary, we now have one more indirection, namely a function self._f() returning a test object, and as indicated in the comments, we're at times compiling and running in separate test objects which morally ought to be the same (and were, previously).
@potsrevennil Could you think about how we can remove indirections rather than adding more?
potsrevennil
force-pushed
the
fix-tests
branch
2 times, most recently
from
377e196 to
c909fed
Compare
|
@hanno-becker thanks for the comment! |
|
@potsrevennil Thank you for the explanations, which I am still to read closely. On first though, we now have |
Signed-off-by: Thing-han, Lim <[email protected]>
Signed-off-by: Thing-han, Lim <[email protected]>
Merge the call for compile and run_schemes into `Test_Implementations.test` function, thus remove the repeated `if compile ... if run ...` part in test functions Signed-off-by: Thing-han, Lim <[email protected]>
Signed-off-by: Thing-han, Lim <[email protected]>
potsrevennil
force-pushed
the
fix-tests
branch
from
c909fed to
9b20d6d
Compare
|
@hanno-becker sorry, the naming was pretty bad, I updated it now |
|
It seems this is no longer strictly necessary after #407 got merged. |
No description provided.