fix and feature: clean MVMextractmode#381
Merged
Merged
Conversation
DasVinch
force-pushed
the
vd/clean_mvm
branch
2 times, most recently
from
July 22, 2026 08:39
d72e15d to
e813048
Compare
Member
Author
|
I've rebased the entire thing - its now an overwrite of the actual code in linearalgebra and is ready to merge. |
DasVinch
marked this pull request as ready for review
July 22, 2026 09:46
There was a problem hiding this comment.
Pull request overview
This PR modernizes the linalgebra MVM mode extraction/expansion compute unit by rewriting MVMextractModes in C++ with pluggable backends (CPU/OMP, BLAS, CUDA/cuBLAS, CUDA-stream/graph), while also adding a new testing/ Python harness (pytest + nox) intended to support coverage and future correctness testing.
Changes:
- Replaces
plugins/milk-extra-src/linalgebra/MVMextractModes.cwith a new C++ implementation (MVMextractModes.cpp) plus backend helpers (mvm_auxiliaries.*). - Adds a
testing/package containing pytest fixtures, placeholder tests, and a nox session for coverage workflows. - Minor infrastructure updates (CMake C++ standard flags,
.clang-format, BLAS/LAPACKE header notes,fps.hPATH_MAX usage).
Reviewed changes
Copilot reviewed 23 out of 27 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| testing/tests/trivial/first_test.py | Adds a trivial pytest to exercise a milk binary (currently needs to be made robust for CI). |
| testing/tests/trivial/init.py | Initializes trivial test package. |
| testing/tests/mains/experiment_deploy.py | Adds a heavy/manual pytest “main” experiment script for streamDelay + MVM benchmarking/correctness. |
| testing/tests/mains/init.py | Initializes mains test package. |
| testing/tests/conftestaux/milk.py | Adds autouse session fixture to spoof MILK_SHM_DIR/MILK_PROC_DIR for tests. |
| testing/tests/conftestaux/coverage.py | Adds session-end gcov dump fixture (if pyMilk wrapper supports it). |
| testing/tests/conftestaux/cacao_loop.py | Placeholder for future cacao loop fixtures. |
| testing/tests/conftestaux/init.py | Initializes conftestaux package. |
| testing/tests/conftest.py | Registers pytest plugin fixtures and adds cwd-to-temp autouse fixture. |
| testing/tests/conftest_test.py | Adds sanity checks that fixtures actually ran. |
| testing/tests/init.py | Initializes tests package. |
| testing/setup.py | Adds a setuptools+CMake-based build wrapper for the testing package. |
| testing/pyproject.toml | Adds pytest config and pyright config for the testing package (ignore pattern currently incorrect). |
| testing/noxfile.py | Adds nox automation for building coverage and running pytest/gcovr. |
| testing/lazymake.sh | Convenience script for local build/copy of shared libs (dev helper). |
| testing/clean.sh | Convenience cleanup script for local pyMilk builds (dev helper). |
| src/engine/libmilkcommon/milk_blas_lapacke.h | Adds BLAS_LIB string macro + C++ include note for LAPACKE header usage. |
| src/engine/libfps/fps.h | Adds PATH_MAX include and uses PATH_MAX for tmux executable path buffer sizing. |
| plugins/milk-extra-src/linalgebra/MVMextractModes.hpp | New header for CLI registration symbol (currently has a C++ guard typo). |
| plugins/milk-extra-src/linalgebra/MVMextractModes.cpp | New C++ FPS compute unit implementation with runtime backend selection. |
| plugins/milk-extra-src/linalgebra/MVMextractModes.c | Removes the prior C implementation. |
| plugins/milk-extra-src/linalgebra/mvm_auxiliaries.hpp | Adds backend class interfaces for CPU/BLAS/CUDA/cuBLAS/CUDA-graph. |
| plugins/milk-extra-src/linalgebra/mvm_auxiliaries.cpp | Adds backend implementations (CPU/OMP, BLAS sgemv, cuBLAS, CUDA graph). |
| plugins/milk-extra-src/linalgebra/linalgebra.c | Removes a weak “test” CLI hook registration. |
| plugins/milk-extra-src/linalgebra/CMakeLists.txt | Expands module sources to include .cpp/.hpp and switches standalone to build from .cpp. |
| CMakeLists.txt | Sets C++ standard flags to -std=c++17 (instead of -std=gnu17). |
| .clang-format | Adds IndentExternBlock: NoIndent to improve formatting around extern "C". |
Comment on lines
+9
to
+12
| #ifdef __cpluscplus | ||
| extern "C" | ||
| { | ||
| #endif |
Comment on lines
+15
to
+17
| #ifdef __cpluscplus | ||
| } | ||
| #endif |
Comment on lines
+409
to
+413
| if (backend == nullptr) | ||
| { | ||
| // TODO print a very explicit error message and fail | ||
| // enum error? exit(0) | ||
| } |
Comment on lines
+455
to
+459
| /* Type conversion to float (all backends) */ | ||
| if (imgid_in.md->datatype != _DATATYPE_FLOAT) | ||
| { | ||
| cast_to_float(imgin_float_casted_ptr, imgid_in.im, n_pixels_spatial_side); | ||
| } |
Comment on lines
+461
to
+466
| imgid_out.md->write = | ||
| 1; // We don't really know at which point the backend workflow will begin writes, so we flag early. | ||
|
|
||
| backend->matrixMul(); // Here we MVM | ||
| // We're done | ||
| processinfo_update_output_stream(processinfo, imgid_out.im, imgid_in.im); |
Comment on lines
+24
to
+27
| addopts = [ | ||
| "--import-mode=importlib", | ||
| "--ignore=mains/*", | ||
| ] |
Comment on lines
+8
to
+14
| import subprocess | ||
|
|
||
|
|
||
| def test_a_trivial_thing(): | ||
| subprocess.run(["milk-fps-list"]) | ||
|
|
||
| assert True |
| #ifndef FPS_H | ||
| #define FPS_H | ||
|
|
||
| #include <linux/limits.h> // For PATH_MAX (but maybe would belong in a global milk compilation header?) // For some reason this fails only for standalone execs, and even not all of them!! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
THIS HAS BEEN REBASE AND IS MERGEABLE (2026-07-22)
I've rebased the entire thing - its now an overwrite of the actual code in linearalgebra and is ready to merge.
MVM -> C++
Capabilities: CPU-plain, BLAS, CUDA CPU-ordered, CUDA with stream.
Building on GPUindex 99 for CPU:
Minor updates to streamDelay still there.
Stubs for a pytest test suite.
For now its a bit of "it runs on my machine (TM)" since the suite is entirely dependent on the latest pyMilk developments.
And the tests of interest here are not integrated in an auto test-suite, but bundled in the tests.
FYI - I run them with
for future ref if anyone reads this:
THIS IS A DEMO PR WITH NO INTENTION TO MERGE AS AS
This PR is a re-write of
MVMextractModes.cthat showcases a number of interesting approaches for future development.Summary of changes:
Refactor
linalgebra/MVMextractModes.cto finish removing unused or irrelevant attributes. Sanitize a few segfaults, remove references to fetching images into milk-data by ImageID. [must go to upstream]Remove test file that was not actually testing our production code [upstream]
There's also some fixing to
streamDelay.cthat got bundled here through a few merges.Create a separate module linalgebra2 really just for
linalgebra2/MVMextractModes.cppas a workspaceCreate a test environment adapted to pytest used in conjuction with pyMilk under
/testingWhat I showcase
C++
restrict/__restrictand (char*) forceful string literal casts)backend::matrixMul()has a clear API contract: take data from the float-casted input array and write it to the output stream. Seemvm_auxiliaries.hpp|cppnullptr, it's much easier to keep track.TEST SUITE
testing/is configured as a python module, for tests.testing/testsis a pytest suite (that contains nothing meaningful but the layout) that can be run with/testing>$ pytesttesting/tests/mainsfiles are not executed by default, but pytest can be used to invoke them and run dev sessions. This is what I did for now, in a single fileexperiment_testing.Timings 1/3 better on CPU (mostly because masking got factored in), a few % on GPU.
What's not great (in this impementation of the MVM): error checking.
Nits: