finetune: remove unused includes

[danbev]

This commit removes unused includes from finetune.cpp.

[cebtenzzre]

Other warnings from iwyu-tool:

/home/jared/src/forks/llama.cpp/examples/finetune/finetune.cpp should add these lines:
#include <cerrno>        // for errno
#include <cmath>         // for sqrtf
#include <cstdint>       // for uint32_t, int64_t, uint8_t, int32_t, SIZE_MAX
#include <cstdio>        // for printf, fprintf, snprintf, NULL, stderr, fclose
#include <cstdlib>       // for exit, srand

See also #3920 which was merged without the include fixes.

[danbev]

@cebtenzzre I'd be happy to add the missing includes reported by iwyu if that is acceptable (not knowing the reason for the original commit to be rejected).

One things I noticed is that when I run iwyu locally it suggests including c header files instead of the cname ones like in your comment above. For example:

$ include-what-you-use --version
include-what-you-use 0.21 (git:dbecade) based on clang version 17.0.6 (Fedora 17.0.6-1.fc39)

$ include-what-you-use -std=c++11 -I. -I/usr/lib/clang/17/include/ -Icommon examples/finetune/finetune.cpp

examples/finetune/finetune.cpp should add these lines:
#include <errno.h>       // for errno
#include <math.h>        // for sqrtf
#include <stdlib.h>      // for exit, srand
#include <cstdint>       // for uint32_t, int64_t, uint8_t, int32_t, SIZE_MAX
#include <cstdio>        // for printf, fprintf, NULL, stderr, snprintf, fclose

examples/finetune/finetune.cpp should remove these lines:

The full include-list for examples/finetune/finetune.cpp:
#include <errno.h>       // for errno
#include <math.h>        // for sqrtf
#include <stdlib.h>      // for exit, srand
#include <algorithm>     // for replace
#include <cstdint>       // for uint32_t, int64_t, uint8_t, int32_t, SIZE_MAX
#include <cstdio>        // for printf, fprintf, NULL, stderr, snprintf, fclose
#include <cstring>       // for size_t, strlen, memcmp, strerror
#include <ctime>         // for time
#include <string>        // for basic_string, operator==, char_traits, string
#include <vector>        // for vector
#include "common.h"      // for die_fmt, die
#include "ggml-alloc.h"  // for ggml_allocr_alloc, ggml_allocr_free, ggml_al...
#include "ggml.h"        // for ggml_tensor, ggml_get_name, ggml_new_tensor_2d
#include "llama.h"       // for llama_get_model_tensor, llama_free, llama_fr...
#include "train.h"       // for assert_shape_2d, train_params_common, copy_t...
---

Do you know if there is some option to iwyu that I've not specified which might be causing this?

[cebtenzzre]

I'd be happy to add the missing includes reported by iwyu if that is acceptable (not knowing the reason for the original commit to be rejected).

I believe it was only rejected because it became out of date and was considered a low priority. Not sure, @ggerganov

One things I noticed is that when I run iwyu locally it suggests including c header files instead of the cname ones like in your comment above. For example:

Yep, I actually edited the output to avoid confusion. iwyu is silly sometimes - you should always use the C++ headers in C++ code, and should never include internal system headers from e.g. "bits" or "asm". But overall it's good at leading you in the right direction.

Btw, if you configure with cmake -B build and then use iwyu-tool -p build, you can automatically run iwyu on the whole project.

[ggerganov]

Sorry, wasn't rejected - just forgot to merge it

[cebtenzzre]

Sorry, wasn't rejected - just forgot to merge it

I'm referring to the commit that I was told to revert: #3920 (review)

[ggerganov]

Ah, it seems to add a lot of unused headers - I don't see the point. Why does the tool report that they are needed?

[cebtenzzre]

Ah, it seems to add a lot of unused headers - I don't see the point. Why does the tool report that they are needed?

One thing about C/C++ is that headers are a messy way to expose an API - headers often depend on other headers, even in the case of system headers, and you can see this if you ever compile with e.g. gcc -H. To a C/C++ source file these are transient dependencies, which are usually considered an implementation detail that could change at any time. I believe modules improve this situation, but they're a C++20 thing and I haven't seen them used in the wild yet.

include-what-you-use finds the headers that a given source file or header directly depends upon (its output notes the symbols that each header is included for), and recommends that you include them directly, both for correctness and such that your code is less likely to break if you compile with a toolchain version which has a different dependency structure in its headers. It also identifies headers that can be removed or replaced with simple forward declarations, to speed up compilation time.

For example, a correct C++ program includes <string> if it uses std::string. But on my machine, <iostream> incidentally pulls in that header. Also, a correct C++ program includes <vector> if it uses std::vector, but on my machine, <queue> incidentally pull in that header.

[danbev]

Yep, I actually edited the output to avoid confusion. iwyu is silly sometimes - you should always use the C++ headers in C++ code, and should never include internal system headers from e.g. "bits" or "asm". But overall it's good at leading you in the right direction.

Ah, good to know. Thanks!

Btw, if you configure with cmake -B build and then use iwyu-tool -p build, you can automatically run iwyu on the whole project.

Nice, I'll try that 👍

[ggerganov]

@cebtenzzre Thanks for the explanation. I think objectively the commit was fine, but somehow I'm hesitant to apply it in such a large step and I have some doubts about the correctness of the suggested changes. For example <map> is definitely not needed in tests/test-llama-grammar.cpp - not sure why it was suggested.

In general, I'm interested in removing unused STL headers, especially from our header files as these have bigger impact so that part of the change is good. But pre-emptively adding includes does not seem OK - prefer to add on case-by-case basis where our CI detects a problem or people report a missing header

[cebtenzzre]

For example <map> is definitely not needed in tests/test-llama-grammar.cpp - not sure why it was suggested.

It is used, because std::map<A, B>::operator[]() is called here:

llama.cpp/tests/test-llama-grammar.cpp

Lines 101 to 104 in c75ca5d

	for (auto pair : expected)
	{
	parsed_grammar.symbol_ids[pair.first] = pair.second;
	}

You could argue that <map> must be included already for the type of symbol_ids, but if it was a pointer and not a value, the type could have been forward-declared instead. The C way of looking at this is that symbol_ids is just an address passed as the first argument to the operator, so without knowing that it's a value in the struct (iwyu doesn't) you need <map> to get the declaration.

Because of include guards, the worst case is that you have an extra line in your source file. include-what-you-use can be a blunt tool at times, but I think its suggestions result in much more consistent #includes.