Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[c++] remove uses of '..' in headers #6409

Merged
merged 9 commits into from
May 4, 2024
Merged

[c++] remove uses of '..' in headers #6409

merged 9 commits into from
May 4, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Apr 8, 2024 •
edited
Loading

Contributes to #5957
Contributes to #6261
Contributes to #6379

Some of the project's headers use relative paths:

LightGBM/include/LightGBM/utils/common.h

Lines 33 to 34 in 28536a0

#include "../../../external_libs/fast_double_parser/include/fast_double_parser.h"
#include "../../../external_libs/fmt/include/fmt/format.h"

This causes problems for anyone wanting to use and install lib_lightgbm.so + LightGBM headers to build their own program. Like this:

In file included from /tmp/lightgbm/include/LightGBM/config.h:21:
/tmp/lightgbm/include/LightGBM/utils/common.h:33:10: fatal error: '../../../external_libs/fast_double_parser/include/fast_double_parser.h' file not found
#include "../../../external_libs/fast_double_parser/include/fast_double_parser.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

As of the changes on this branch, make install or similar with LightGBM will install the headers with the following layout:

-- Install configuration: ""
-- Installing: /tmp/lightgbm/bin/lightgbm
-- Installing: /tmp/lightgbm/lib/lib_lightgbm.dylib
-- Installing: /tmp/lightgbm/include/LightGBM
-- Installing: /tmp/lightgbm/include/LightGBM/objective_function.h
-- Installing: /tmp/lightgbm/include/LightGBM/prediction_early_stop.h
-- Installing: /tmp/lightgbm/include/LightGBM/export.h
-- Installing: /tmp/lightgbm/include/LightGBM/config.h
-- Installing: /tmp/lightgbm/include/LightGBM/metric.h
-- Installing: /tmp/lightgbm/include/LightGBM/c_api.h
-- Installing: /tmp/lightgbm/include/LightGBM/application.h
-- Installing: /tmp/lightgbm/include/LightGBM/arrow.tpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_metadata.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_split_info.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_utils.hu
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/vector_cudahost.h
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_algorithms.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_objective_function.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_metric.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_random.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_column_data.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_tree.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_row_data.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/tree.h
-- Installing: /tmp/lightgbm/include/LightGBM/dataset_loader.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils
-- Installing: /tmp/lightgbm/include/LightGBM/utils/chunked_array.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/utils/array_args.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/file_io.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/threading.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/pipeline_reader.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/openmp_wrapper.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/json11.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/common.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/binary_writer.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/log.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/text_reader.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/yamc
-- Installing: /tmp/lightgbm/include/LightGBM/utils/yamc/yamc_shared_lock.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/utils/yamc/alternate_shared_mutex.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/utils/yamc/yamc_rwlock_sched.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/utils/byte_buffer.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/random.h
-- Installing: /tmp/lightgbm/include/LightGBM/dataset.h
-- Installing: /tmp/lightgbm/include/LightGBM/train_share_states.h
-- Installing: /tmp/lightgbm/include/LightGBM/network.h
-- Installing: /tmp/lightgbm/include/LightGBM/tree_learner.h
-- Installing: /tmp/lightgbm/include/LightGBM/feature_group.h
-- Installing: /tmp/lightgbm/include/LightGBM/meta.h
-- Installing: /tmp/lightgbm/include/LightGBM/arrow.h
-- Installing: /tmp/lightgbm/include/LightGBM/boosting.h
-- Installing: /tmp/lightgbm/include/LightGBM/sample_strategy.h
-- Installing: /tmp/lightgbm/include/LightGBM/bin.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fast_double_parser.h
-- Up-to-date: /tmp/lightgbm/include/LightGBM/utils
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/ostream.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/format-inl.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/ranges.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/xchar.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/core.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/chrono.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/os.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/color.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/args.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/printf.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/compile.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/format.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/std.h

How I tested this

Compiled a small test C++ program interacting with LightGBM through its C API.

details (click me)

On my mac, tried building the library and installing it to /tmp/lightgbm, like this:

cmake \
    -B build \
    -S . \
    -DUSE_OPENMP=OFF \
    -DCMAKE_INSTALL_PREFIX=/tmp/lightgbm

cmake --build build --target install -j4

Compilation succeeded (in this case, using AppleClang 15) and the library and its headers were installed to /tmp/lightgbm.

[  2%] Building CXX object CMakeFiles/lightgbm_capi_objs.dir/src/c_api.cpp.o
...
[ 97%] Linking CXX shared library /Users/jlamb/repos/LightGBM/lib_lightgbm.dylib
[ 97%] Built target _lightgbm
[100%] Linking CXX executable /Users/jlamb/repos/LightGBM/lightgbm
[100%] Built target lightgbm
Install the project...
-- Install configuration: ""
-- Installing: /tmp/lightgbm/bin/lightgbm
-- Installing: /tmp/lightgbm/lib/lib_lightgbm.dylib
-- Installing: /tmp/lightgbm/include/LightGBM
-- Installing: /tmp/lightgbm/include/LightGBM/objective_function.h
-- Installing: /tmp/lightgbm/include/LightGBM/prediction_early_stop.h
-- Installing: /tmp/lightgbm/include/LightGBM/export.h
-- Installing: /tmp/lightgbm/include/LightGBM/config.h
-- Installing: /tmp/lightgbm/include/LightGBM/metric.h
-- Installing: /tmp/lightgbm/include/LightGBM/c_api.h
-- Installing: /tmp/lightgbm/include/LightGBM/application.h
-- Installing: /tmp/lightgbm/include/LightGBM/arrow.tpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_metadata.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_split_info.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_utils.hu
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/vector_cudahost.h
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_algorithms.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_objective_function.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_metric.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_random.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_column_data.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_tree.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/cuda/cuda_row_data.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/tree.h
-- Installing: /tmp/lightgbm/include/LightGBM/dataset_loader.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils
-- Installing: /tmp/lightgbm/include/LightGBM/utils/chunked_array.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/utils/array_args.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/file_io.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/threading.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/pipeline_reader.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/openmp_wrapper.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/json11.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/common.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/binary_writer.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/log.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/text_reader.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/yamc
-- Installing: /tmp/lightgbm/include/LightGBM/utils/yamc/yamc_shared_lock.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/utils/yamc/alternate_shared_mutex.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/utils/yamc/yamc_rwlock_sched.hpp
-- Installing: /tmp/lightgbm/include/LightGBM/utils/byte_buffer.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/random.h
-- Installing: /tmp/lightgbm/include/LightGBM/dataset.h
-- Installing: /tmp/lightgbm/include/LightGBM/train_share_states.h
-- Installing: /tmp/lightgbm/include/LightGBM/network.h
-- Installing: /tmp/lightgbm/include/LightGBM/tree_learner.h
-- Installing: /tmp/lightgbm/include/LightGBM/feature_group.h
-- Installing: /tmp/lightgbm/include/LightGBM/meta.h
-- Installing: /tmp/lightgbm/include/LightGBM/arrow.h
-- Installing: /tmp/lightgbm/include/LightGBM/boosting.h
-- Installing: /tmp/lightgbm/include/LightGBM/sample_strategy.h
-- Installing: /tmp/lightgbm/include/LightGBM/bin.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fast_double_parser.h
-- Up-to-date: /tmp/lightgbm/include/LightGBM/utils
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/ostream.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/format-inl.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/ranges.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/xchar.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/core.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/chrono.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/os.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/color.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/args.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/printf.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/compile.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/format.h
-- Installing: /tmp/lightgbm/include/LightGBM/utils/fmt/std.h

Ideally, a program wanting to use lib_lightgbm.so should be able to just include LightGBM/c_api.h to link to it. I tried with such a program, like this:

// main.cpp
#include <string>

#include <LightGBM/c_api.h>
#include <LightGBM/dataset.h>

int main(int argc, char *argv[])
{
    const char* config = "boosting=gbdt objective=regression num_iterations=5 verbose=1";

    // hard-coding a path assuming this will be run from the root of the LightGBM repo, just for simplicity
    std::string train_data_file("examples/regression/regression.train");

    BoosterHandle booster_handle = nullptr;
    DatasetHandle dataset_handle = nullptr;

    LGBM_DatasetCreateFromFile(
        train_data_file.c_str(),
        config,
        nullptr,
        &dataset_handle
    );

    LGBM_BoosterCreate(
        dataset_handle,
        config,
        &booster_handle
    );

    // train for 5 iterations
    int is_finished = 0;
    for(int iter=1;iter<5;iter++){
        LGBM_BoosterUpdateOneIter(
            booster_handle,
            &is_finished
        );
    }

    // write model file
    std::string model_file("model_exmple.txt");
    LGBM_BoosterSaveModel(
        booster_handle,
        0, // start_iteration
        -1, // num_iteration
        C_API_FEATURE_IMPORTANCE_SPLIT,
        model_file.c_str()
    );
}

Attempted to compile it like this:

clang++ \
    -std=c++11 \
    -o ./lgb-train \
    -I/tmp/lightgbm/include \
    -l_lightgbm \
    -L/tmp/lightgbm/lib \
    -Wl,-rpath,/tmp/lightgbm/lib \
    ./main.cpp

That fails like this:

In file included from /tmp/lightgbm/include/LightGBM/config.h:21:
/tmp/lightgbm/include/LightGBM/utils/common.h:33:10: fatal error: '../../../external_libs/fast_double_parser/include/fast_double_parser.h' file not found
#include "../../../external_libs/fast_double_parser/include/fast_double_parser.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

On this branch, it succeeds, and the program runs successfully 😁

./lgb-train
[LightGBM] [Info] Loading initial scores...
[LightGBM] [Info] Construct bin mappers from text data time 0.03 seconds
[LightGBM] [Info] Auto-choosing row-wise multi-threading, the overhead of testing was 0.000382 seconds.
You can set `force_row_wise=true` to remove the overhead.
And if memory is not enough, you can set `force_col_wise=true`.
[LightGBM] [Info] Total Bins 6132
[LightGBM] [Info] Number of data points in the train set: 7000, number of used features: 28

Notes for Reviewers

Why only fast_double_parser and fmt? Isn't this a problem for Boost (external_libs/compute/) and Eigen (external_libs/Eigen)?

For using LightGBM's C API, only fast_double_parser and fmt headers need to be available to #include <LightGBM/c_api.h>.

@jameslamb jameslamb changed the title [c++] remove uses of '..' in headers WIP: [c++] remove uses of '..' in headers Apr 8, 2024
@jameslamb jameslamb changed the title WIP: [c++] remove uses of '..' in headers [c++] remove uses of '..' in headers May 1, 2024
@jameslamb jameslamb marked this pull request as ready for review May 1, 2024 04:09
@jameslamb jameslamb mentioned this pull request May 1, 2024
Merged
33 tasks
borchero
borchero approved these changes May 1, 2024
View reviewed changes
Copy link
Collaborator

@borchero borchero left a comment

Choose a reason for hiding this comment

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

I have little CMake knowledge but no relative paths makes sense to me 😄

@jameslamb
Copy link
Collaborator Author

I have little CMake knowledge but no relative paths makes sense to me 😄

No problem, thanks for taking a look! I've spent a lot of time over the last 6 months getting more familiar with CMake, hopefully will be able to bring more simplifications like this one here.

To be sure about this, let's get another review from either @guolinke or @shiyu1994 before merging this.

guolinke
guolinke approved these changes May 4, 2024
View reviewed changes
Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

Thank you, looks good to me!

@jameslamb
Copy link
Collaborator Author

Thank you!

@jameslamb jameslamb merged commit 6e78e69 into master May 4, 2024
38 checks passed
@jameslamb jameslamb deleted the fix/include-paths branch May 4, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guolinke guolinke guolinke approved these changes

@borchero borchero borchero approved these changes

@shiyu1994 shiyu1994 Awaiting requested review from shiyu1994 shiyu1994 is a code owner

@jmoralez jmoralez Awaiting requested review from jmoralez jmoralez is a code owner

Assignees
No one assigned
Labels
maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jameslamb @guolinke @borchero