Skip to content

train : fix KQ_pos allocation#3392

Merged
ggerganov merged 2 commits into
masterfrom
train-fix-kq-pos
Sep 29, 2023
Merged

train : fix KQ_pos allocation#3392
ggerganov merged 2 commits into
masterfrom
train-fix-kq-pos

Conversation

@ggerganov

Copy link
Copy Markdown
Member

fix #3389

#3228 changes seem to have broken the train examples. I think this should fix it

@ggerganov
ggerganov requested a review from xaedes September 29, 2023 08:52
@xaedes

xaedes commented Sep 29, 2023

Copy link
Copy Markdown
Collaborator

I am currently letting it run a test finetune & train to see if it actually works, but from looking at it I think this should be correct.

On a sitenote, a function ggml_range(ctx, dtype, start, stop, step) analog to numpy.arange would be nice to have for filling a tensor with a sequence of values.
It is a obvious basic primitive for array/tensor processing libs and would avoid the need for this manual for loop.

struct ggml_tensor * KQ_pos = ggml_new_tensor_1d(ctx, GGML_TYPE_I32, N);
ggml_allocr_alloc(alloc, KQ_pos);
if (!ggml_allocr_is_measure(alloc)) {
    int * data = (int *) KQ_pos->data;
    for (int i = 0; i < N; ++i) {
        data[i] = n_past + i;
    }
}

would then just look like this:

struct ggml_tensor * KQ_pos = ggml_range(ctx, GGML_TYPE_I32, 0, N, 1);
ggml_allocr_alloc(alloc, KQ_pos);

@ggerganov

Copy link
Copy Markdown
Member Author

Yup, ggml_range() is a great idea - we will add it

@xaedes

xaedes commented Sep 29, 2023

Copy link
Copy Markdown
Collaborator

Yup, ggml_range() is a great idea - we will add it

Ok, after testing the train & finetune from this PR here, I will make a PR for ggml_range.

@slaren

slaren commented Sep 29, 2023

Copy link
Copy Markdown
Member

ggml_range looks useful, but as it is now, implementing it would require adding an implementation in every backend.

@xaedes xaedes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Making sure that KQ_pos is not reallocated was missing in finetune.

Performed some finetune and train tests, the results indicate that it works.

@ggerganov
ggerganov merged commit bc34dd4 into master Sep 29, 2023
@ggerganov
ggerganov deleted the train-fix-kq-pos branch September 29, 2023 16:05
joelkuiper added a commit to vortext/llama.cpp that referenced this pull request Oct 2, 2023
…example

* 'master' of github.com:ggerganov/llama.cpp:
  ggml-cuda : perform cublas mat mul of quantized types as f16 (ggml-org#3412)
  llama.cpp : add documentation about rope_freq_base and scale values (ggml-org#3401)
  train : fix KQ_pos allocation (ggml-org#3392)
  llama : quantize up to 31% faster on Linux and Windows with mmap (ggml-org#3206)
  readme : update hot topics + model links (ggml-org#3399)
  readme : add link to grammars app (ggml-org#3388)
  swift : fix build on xcode 15 (ggml-org#3387)
  build : enable more non-default compiler warnings (ggml-org#3200)
  ggml_tensor: update the structure comments. (ggml-org#3283)
  ggml : release the requested thread pool resource (ggml-org#3292)
  llama.cpp : split llama_context_params into model and context params (ggml-org#3301)
  ci : multithreaded builds (ggml-org#3311)
  train : finetune LORA (ggml-org#2632)
  gguf : basic type checking in gguf_get_* (ggml-org#3346)
  gguf : make token scores and types optional (ggml-org#3347)
  ci : disable freeBSD builds due to lack of VMs (ggml-org#3381)
  llama : custom attention mask + parallel decoding + no context swaps (ggml-org#3228)
  docs : mark code as Bash (ggml-org#3375)
  readme : add Mistral AI release 0.1 (ggml-org#3362)
  ggml-cuda : perform cublas fp16 matrix multiplication as fp16 (ggml-org#3370)
yusiwen pushed a commit to yusiwen/llama.cpp that referenced this pull request Oct 7, 2023
* train : fix KQ_pos allocation

* make sure KQ_pos is not reallocated in finetune

---------

Co-authored-by: xaedes <xaedes@gmail.com>
Seunghhon pushed a commit to Seunghhon/llama.cpp that referenced this pull request Apr 26, 2026
* train : fix KQ_pos allocation

* make sure KQ_pos is not reallocated in finetune

---------

Co-authored-by: xaedes <xaedes@gmail.com>
phuongncn pushed a commit to phuongncn/llama.cpp-gx10-dgx-sparks-deepseekv4 that referenced this pull request Apr 28, 2026
* train : fix KQ_pos allocation

* make sure KQ_pos is not reallocated in finetune

---------

Co-authored-by: xaedes <xaedes@gmail.com>
ljubomirj pushed a commit to ljubomirj/llama.cpp that referenced this pull request May 6, 2026
* train : fix KQ_pos allocation

* make sure KQ_pos is not reallocated in finetune

---------

Co-authored-by: xaedes <xaedes@gmail.com>
AlexiAlp pushed a commit to minghaop/llama.cpp that referenced this pull request Jun 2, 2026
* train : fix KQ_pos allocation

* make sure KQ_pos is not reallocated in finetune

---------

Co-authored-by: xaedes <xaedes@gmail.com>
AlexiAlp pushed a commit to minghaop/llama.cpp that referenced this pull request Jun 2, 2026
* train : fix KQ_pos allocation

* make sure KQ_pos is not reallocated in finetune

---------

Co-authored-by: xaedes <xaedes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

train-text-from-scratch.cpp: dereferenced NULL KQ_pos->data

3 participants