Skip to content

tests : add non-cont K,V FA tests #14756

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 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ggerganov
Copy link
Member

@ggerganov ggerganov commented Jul 18, 2025

cont #14363

With the introduction of a split KV cache, the K and V tensors passed to FA can now be non-contiguous. Add tests in test-backend-ops to cover this.

This issue was reported here: #14363 (comment)

It can be reproduced with this command with CUDA backend:

make -j && LLAMA_SET_ROWS=1 ./bin/llama-parallel -hf ggml-org/Qwen2.5-Coder-3B-Q8_0-GGUF -np 8 -ns 128 -s 1 -c 4096 -fa -ngl 99 --top-k 1 -ctk q8_0 -ctv q8_0
0.02.205.072 I common_init_from_params: setting dry_penalty_last_n to ctx_size = 4608
0.02.205.072 W common_init_from_params: warming up the model with an empty run - please wait ... (--no-warmup to disable)
0.02.233.146 I No new questions so proceed with build-in defaults.
0.02.233.146 I 

0.02.240.868 I main: Simulating parallel requests from clients:
0.02.240.870 I main: n_parallel = 8, n_sequences = 128, cont_batching = 1, system tokens = 273
0.02.240.870 I 
0.02.240.870 I Processing requests ...

0.02.241.045 I main: clearing the KV cache
0.02.248.040 I Client   0, seq    0, junk =    0, prompt = 284, started decoding ...
0.02.254.999 I Client   1, seq    1, junk =    0, prompt = 284, started decoding ...
0.02.262.112 I Client   2, seq    2, junk =    0, prompt = 284, started decoding ...
0.02.269.266 I Client   3, seq    3, junk =    0, prompt = 290, started decoding ...
0.02.276.355 I Client   4, seq    4, junk =    0, prompt = 288, started decoding ...
0.02.283.337 I Client   5, seq    5, junk =    0, prompt = 285, started decoding ...
0.02.290.405 I Client   6, seq    6, junk =    0, prompt = 286, started decoding ...
0.02.297.367 I Client   7, seq    7, junk =    0, prompt = 284, started decoding ...
/home/ggerganov/development/github/llama.cpp/ggml/src/ggml-cuda/template-instances/../fattn-common.cuh:748: GGML_ASSERT(ggml_is_contiguously_allocated(K)) failed

cc @JohannesGaessler

@github-actions github-actions bot added the testing Everything test related label Jul 18, 2025
@ggerganov ggerganov mentioned this pull request Jul 18, 2025
23 tasks
Copy link

@OrangeDoro OrangeDoro left a comment

Choose a reason for hiding this comment

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

Hi! I'm a grad student working on a research project about using large language models to automate code review. Based on your commit a856a56 and the changes in tests/test-backend-ops.cpp, my tool generated this comment:

  1. Error Handling: Implement error handling for tensor creation functions. Check if ggml_new_tensor_4d or ggml_view_4d returns a null pointer to prevent dereferencing null pointers later in the code.
  2. Null Pointer Checks: The code does not check if the ctx pointer is null before passing it to functions like ggml_new_tensor_4d, ggml_view_4d, and ggml_permute. It is advisable to add a check for ctx at the beginning of the create_permuted function.
  3. Input Validation: The function does not validate the dimensions passed to ggml_new_tensor_4d or ggml_view_4d. Consider adding checks to ensure that the dimensions are within acceptable bounds before proceeding with tensor creation.
  4. Parameter Addition: Ensure that the calling code correctly specifies the is_view parameter for each tensor creation.
  5. Tensor Creation Logic: Verify the doubling of ne_perm[1] when is_view is true to ensure it is the intended behavior. If this is conditional, it should be documented or validated.
  6. Variable Initialization: Initialize the variable ggml_tensor * t; to nullptr to avoid potential undefined behavior.
  7. Memory Management: The code creates a tensor t0 when is_view is true, but it does not appear to free t0 after it is used to create the view t. Ensure that there is a mechanism to free t0 when it is no longer needed.
  8. Redundant Tensor Creation Logic: The logic for creating a tensor is duplicated in the if and else branches of the is_view check. This could be refactored to reduce redundancy.
  9. Concurrency Issues: If this code is executed in a multi-threaded environment, ensure that the context ctx is thread-safe.
  10. Test Coverage for create_permuted: Add unit tests for create_permuted with is_view set to true to verify that the tensor is created correctly as a view.

As part of my research, I'm trying to understand how useful these comments are in real-world development. If you have a moment, I'd be super grateful if you could quickly reply to these two yes/no questions:

  1. Does this comment provide suggestions from a dimension you hadn’t considered?

  2. Do you find this comment helpful?

Thanks a lot for your time and feedback! And sorry again if this message is a bother.

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.

2 participants