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.

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

feat: optimization for oneAPI on windows #911

Closed
Tracked by #905
tikikun opened this issue Dec 8, 2023 · 6 comments
Closed
Tracked by #905

feat: optimization for oneAPI on windows #911

tikikun opened this issue Dec 8, 2023 · 6 comments
Assignees
Labels

Comments

@tikikun
Copy link
Contributor

tikikun commented Dec 8, 2023

Problem
Currently the windows jan does not support oneAPI which will not provide the best performance for intel hardware

Alan successfully built Nitro with OneAPI using this docker environment:

# Use Debian stable slim as the base image
FROM intel/oneapi-basekit:2024.0.0-devel-ubuntu22.04

# Set working directory
WORKDIR /app

# Install required packages
RUN apt-get update && \
    apt-get install -y git cmake numactl uuid-dev && \
    git clone --recurse https://github.com/janhq/nitro && \
    cd nitro && \
    ./install_deps.sh && \
    mkdir build && \
    cd build && \
    cmake .. -DLLAMA_BLAS=ON -DLLAMA_BLAS_VENDOR=Intel10_64lp -DCMAKE_C_COMPILER=icx -DCMAKE_CXX_COMPILER=icpx && \
    cmake --build . --config Release -j $(nproc) && \
    apt-get remove --purge -y git cmake && \
    apt-get autoremove -y && \
    apt-get clean && \
    rm -rf /var/lib/apt/lists/*

# Expose the port
EXPOSE 3928

# Change the permissions of the nitro binary to make it executable
RUN chmod  x /app/nitro/build/nitro

# Set the command to run the nitro binary with numactl limiting to cores 0-7
ENTRYPOINT ["numactl", "--physcpubind=0-7", "/app/nitro/build/nitro"]
CMD ["1", "0.0.0.0", "3928"]

However the result binary does not work with system with without OneAPI runtime, tested on Debian base image.
We need to do find a way to deliver this Nitro binary, with focus on Windows first, and later extends to other OS.

Success Criteria
Figure out a way to include OneAPI to windows. Either by statically compile OneAPI into Nitro binary, or delivering minimal OneAPI run-time with Nitro installation.

Alan added comment: We need to have the nitro binary independence (no need for oneAPI runtime) or a good way to install oneAPI runtime to customer machine.

@tikikun tikikun added the type: feature request A new feature label Dec 8, 2023
@tikikun tikikun self-assigned this Dec 8, 2023
@dan-jan dan-jan added this to the 0.4.1: Jan on Windows milestone Dec 11, 2023
@linhtran174 linhtran174 self-assigned this Dec 12, 2023
@linhtran174
Copy link
Contributor

  • OneAPI on Windows requires integration with Visual Studio build tools (only compatible version only).
    I'm testing with Visual Studio 2019 - MSVC/14.29.30133. The installation is a bit tricky, esp if you have pre-installed VS build tool before Visual Studio, so I recommend installing on a fresh PC with fresh VS installation.

  • Recommended compiler set is different from Linux (theoretically still work across platforms):
    -- Linux C: icx, C : icpx
    -- Windows C: icx-cc, C icpx

Got Cmake FindBLAS to work, still resolving other problems:

-- Configuring incomplete, errors occurred!
PS C:\Users\linh\nitro\build> cmake .. -DLLAMA_BLAS=ON -DLLAMA_BLAS_VENDOR=Intel10_64lp -DCMAKE_C_COMPILER=icx-cc -DCMAKE_CXX_COMPILER=icpx
-- Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.19045.
-- Looking for sgemm_
-- Looking for sgemm_ - found
-- Found BLAS: C:/Program Files (x86)/Intel/oneAPI/mkl/latest/lib/mkl_intel_lp64_dll.lib;C:/Program Files (x86)/Intel/oneAPI/compiler/latest/lib/libiomp5md.lib;C:/Program Files (x86)/Intel/oneAPI/mkl/latest/lib/mkl_intel_thread_dll.lib;C:/Program Files (x86)/Intel/oneAPI/mkl/latest/lib/mkl_core_dll.lib
-- BLAS found, Libraries: C:/Program Files (x86)/Intel/oneAPI/mkl/latest/lib/mkl_intel_lp64_dll.lib;C:/Program Files (x86)/Intel/oneAPI/compiler/latest/lib/libiomp5md.lib;C:/Program Files (x86)/Intel/oneAPI/mkl/latest/lib/mkl_intel_thread_dll.lib;C:/Program Files (x86)/Intel/oneAPI/mkl/latest/lib/mkl_core_dll.lib
CMake Error at C:/Program Files/CMake/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)
Call Stack (most recent call first):
  C:/Program Files/CMake/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files/CMake/share/cmake-3.28/Modules/FindPkgConfig.cmake:99 (find_package_handle_standard_args)
  llama.cpp/CMakeLists.txt:198 (find_package)


-- Configuring incomplete, errors occurred!
PS C:\Users\linh\nitro\build>

@linhtran174
Copy link
Contributor

  • Added vcpkg for Cmake PkgConfig functionality
  • Got an errorneous path to MKL, not sure if Cmake FindBLAS bug or var setting of oneAPI, but gonna hard code the path to get through this step
-- BLAS found, Includes: C:/Program;Files;(x86)/Intel/oneAPI/mkl/latest/include
CMake Error at llama.cpp/CMakeLists.txt:243 (if):
  if given arguments:

    "C:/Program" "Files" "(x86)/Intel/oneAPI/mkl/latest/include" "MATCHES" "mkl" "AND" "(" "Intel10_64lp" "MATCHES" "Generic" "OR" "Intel10_64lp" "MATCHES" "Intel" ")"

  Unknown arguments specified

@tikikun
Copy link
Contributor Author

tikikun commented Dec 13, 2023

why do we need vcpkg on windows? can we do without?

@linhtran174
Copy link
Contributor

I chose vcpkg as PkgConfig replacement for Windows, on first search. Maybe @hiento09 could help, is there better alt?
Also this is build time so doesn't affect end-users. But sure devs on Windows may need extra toolings.

@linhtran174 linhtran174 linked a pull request Dec 13, 2023 that will close this issue
@linhtran174
Copy link
Contributor

MKL doesn't seem do use Intel iGPU, no perf increase
OpenBLAS - normal build
openblas
Intel MKL - oneAPI
mkl

@linhtran174
Copy link
Contributor

linhtran174 commented Dec 14, 2023

Seems we need additional work to get GPU offloading with OneMKL.
ggerganov/llama.cpp#1761

Particularly offloading to OpenMP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants