Skip to content

test-backend-ops: add support for specifying output format #14368

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yeahdongcn
Copy link
Collaborator

@yeahdongcn yeahdongcn commented Jun 25, 2025

Make sure to read the contributing guidelines before submitting a PR

Testing Done

root@xiaodongye-s80:/ws# ./build/bin/test-backend-ops --help
Usage: ./build/bin/test-backend-ops [mode] [-o <op>] [-b <backend>] [-p <params regex>] [--output <console|sql>]
    valid modes:
      - test (default, compare with CPU backend for correctness)
      - grad (compare gradients from backpropagation with method of finite differences)
      - perf (performance evaluation)
    op names for -o are as given by ggml_op_desc() (e.g. ADD, MUL_MAT, etc)
    --output specifies output format (default: console)

root@xiaodongye-s80:/ws# ./build/bin/test-backend-ops perf -b CPU -o ADD
ggml_cuda_init: GGML_CUDA_FORCE_MMQ:    no
ggml_cuda_init: GGML_CUDA_FORCE_CUBLAS: no
ggml_cuda_init: found 1 MUSA devices:
  Device 0: MTT S80, compute capability 2.1, VMM: yes
Testing 2 devices

Backend 1/2: MUSA0
  Skipping
Backend 2/2: CPU
  Device description: 12th Gen Intel(R) Core(TM) i5-12400
  Device memory: 31859 MB (31859 MB free)

  ADD(type=f32,ne=[4096,1,1,1],nr=[1,1,1,1]):   ADD(type=f32,ne=[4096,1,1,1],nr=[1,1,1,1]):                1572480 runs -     0.64 us/run -       48 kB/run -   71.97 GB/s
  ADD(type=f32,ne=[4096,1,1,1],nr=[1,512,1,1]):   ADD(type=f32,ne=[4096,1,1,1],nr=[1,512,1,1]):                13338 runs -    76.12 us/run -    24576 kB/run -  308.22 GB/s
  Backend CPU: OK

2/2 backends passed
OK

root@xiaodongye-s80:/ws# ./build/bin/test-backend-ops perf -b CPU -o ADD --output sql
ggml_cuda_init: GGML_CUDA_FORCE_MMQ:    no
ggml_cuda_init: GGML_CUDA_FORCE_CUBLAS: no
ggml_cuda_init: found 1 MUSA devices:
  Device 0: MTT S80, compute capability 2.1, VMM: yes
CREATE TABLE IF NOT EXISTS test_backend_ops (
  test_time TEXT,
  build_commit TEXT,
  build_number INTEGER,
  backend_name TEXT,
  op_name TEXT,
  op_params TEXT,
  test_mode TEXT,
  supported INTEGER,
  passed INTEGER,
  error_message TEXT,
  time_us REAL,
  flops REAL,
  bandwidth_gb_s REAL,
  memory_kb INTEGER,
  n_runs INTEGER
);

INSERT INTO test_backend_ops (test_time, build_commit, build_number, backend_name, op_name, op_params, test_mode, supported, passed, error_message, time_us, flops, bandwidth_gb_s, memory_kb, n_runs) VALUES ('2025-06-27T05:54:54Z', '1d5f25c53', '5756', 'CPU', 'ADD', 'type=f32,ne=[4096,1,1,1],nr=[1,1,1,1]', 'perf', '1', '1', '', '1.003772', '0.000000', '45.608051', '48', '1007370');
INSERT INTO test_backend_ops (test_time, build_commit, build_number, backend_name, op_name, op_params, test_mode, supported, passed, error_message, time_us, flops, bandwidth_gb_s, memory_kb, n_runs) VALUES ('2025-06-27T05:54:55Z', '1d5f25c53', '5756', 'CPU', 'ADD', 'type=f32,ne=[4096,1,1,1],nr=[1,512,1,1]', 'perf', '1', '1', '', '134.092504', '0.000000', '174.956746', '24576', '7524');

root@xiaodongye-s80:/ws# ./build/bin/test-backend-ops perf -b CPU -o ADD --output sql | sqlite3 add.sqlite
ggml_cuda_init: GGML_CUDA_FORCE_MMQ:    no
ggml_cuda_init: GGML_CUDA_FORCE_CUBLAS: no
ggml_cuda_init: found 1 MUSA devices:
  Device 0: MTT S80, compute capability 2.1, VMM: yes

db

Edit (added build_commit and build_number for future use):

add

@github-actions github-actions bot added the testing Everything test related label Jun 25, 2025
@yeahdongcn yeahdongcn force-pushed the xd/test-backend-ops_sql branch from bfa7a43 to 359d792 Compare June 25, 2025 08:21
@yeahdongcn yeahdongcn requested a review from Copilot June 25, 2025 08:44
Copilot

This comment was marked as outdated.

@yeahdongcn yeahdongcn force-pushed the xd/test-backend-ops_sql branch from 359d792 to 34500f9 Compare June 25, 2025 09:16
@yeahdongcn yeahdongcn requested a review from Copilot June 25, 2025 09:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for specifying the output format (console or SQL) for the test-backend-ops tool and unifies logging via a new printer interface.

  • Introduces a printer interface with concrete implementations for console and SQL output.
  • Updates test evaluation functions and the CLI to use the printer for logging.
  • Adds new command-line option "--output" along with helper functions to parse the output format.

@yeahdongcn yeahdongcn force-pushed the xd/test-backend-ops_sql branch from 34500f9 to 679a141 Compare June 25, 2025 09:46
@yeahdongcn yeahdongcn marked this pull request as ready for review June 25, 2025 09:47
Copy link
Collaborator

@JohannesGaessler JohannesGaessler left a comment

Choose a reason for hiding this comment

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

From my end I think these changes to test-backend-ops would be fine but keep in mind that it's an important piece of the project with many stakeholders.

passed = false;

// Set test time
time_t t = time(NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO

Copy link
Collaborator Author

@yeahdongcn yeahdongcn Jun 26, 2025

Choose a reason for hiding this comment

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

This was added to record the timestamp (the same as llama-bench), but I haven’t decided how to use it.

@yeahdongcn
Copy link
Collaborator Author

From my end I think these changes to test-backend-ops would be fine but keep in mind that it's an important piece of the project with many stakeholders.

Thanks for pointing that out! I’ll add the recent contributors as reviewers.

Comment on lines +41 to +43
// External declarations for build info
extern int LLAMA_BUILD_NUMBER;
extern const char *LLAMA_COMMIT;
Copy link
Member

Choose a reason for hiding this comment

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

test-backend-ops is part of ggml, and cannot depend on llama.cpp.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main goal of this work is to generate a comparison table for test-backend-ops, which I believe is especially useful when updating or optimizing op implementations. For more context, please refer to #14354.

The commit hash is used to compare performance changes between two revisions, as shown in #14392.

Do you have any suggestions on how to update this? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

There are already GGML_BUILD_NUMBER and GGML_BUILD_COMMIT variables in the ggml build. They are currently not passed to the code, but you pass them through a compile_definition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed that the generated build-info.cpp contains the same commit hash and build number for both LLAMA and GGML:

int LLAMA_BUILD_NUMBER = 5757;
char const *LLAMA_COMMIT = "38d4930ec";
char const *LLAMA_COMPILER = "Apple clang version 17.0.0 (clang-1700.0.13.5)";
char const *LLAMA_BUILD_TARGET = "arm64-apple-darwin24.5.0";
int GGML_BUILD_NUMBER = 5757;
char const *GGML_BUILD_COMMIT = "38d4930ec";

Please see the new commit: 76ca4f6 Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

No, this way still depends on the llama.cpp common lib.

Comment on lines 461 to 467
// General purpose output methods
virtual void print_info(const char * format, ...) = 0;
virtual void print_error(const char * format, ...) = 0;
virtual void print_device_info(const char * format, ...) = 0;
virtual void print_test_summary(const char * format, ...) = 0;
virtual void print_status_ok() = 0;
virtual void print_status_fail() = 0;
Copy link
Member

@slaren slaren Jun 30, 2025

Choose a reason for hiding this comment

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

I don't think this a good design, all of these functions do the same and could be replaced with a single print_message. If you want transfer the responsibility of formatting the output to a class, then the class needs to have the information to decide what to print, not an unstructured message. Ideally all of the functions would receive an object to print, and the class would determine how to format that object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. I’ll update the code accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please check the latest commit. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really change anything. The ideal solution would be to remove all the messages entirely, and pass enough information to the printers so that they format the output in any way they want. That would require a significant refactor, and not simply replacing calls to printf with printer->print_message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please check if I understand this correctly: f4f5512
Thanks.

Signed-off-by: Xiaodong Ye <[email protected]>
Signed-off-by: Xiaodong Ye <[email protected]>
@yeahdongcn yeahdongcn force-pushed the xd/test-backend-ops_sql branch from 76ca4f6 to f4f5512 Compare July 2, 2025 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants