gemini-code-assist[bot] commented on code in PR #19656: URL: https://github.com/apache/tvm/pull/19656#discussion_r3338163810
########## pyproject.toml: ########## @@ -34,55 +35,39 @@ classifiers = [ "Intended Audience :: Developers", "Intended Audience :: Education", "Intended Audience :: Science/Research", - "License :: OSI Approved :: Apache Software License", "Programming Language :: Python :: 3", - "Programming Language :: Python :: 3.9", "Programming Language :: Python :: 3.10", "Programming Language :: Python :: 3.11", "Programming Language :: Python :: 3.12", + "Programming Language :: Python :: 3.13", "Topic :: Scientific/Engineering :: Artificial Intelligence", "Topic :: Software Development :: Libraries :: Python Modules", ] -# Core dependencies - these are the minimum required for basic TVM functionality dependencies = [ - "apache-tvm-ffi", - "cloudpickle", + "apache-tvm-ffi>=0.1.11", "ml_dtypes", "numpy", - "packaging", "psutil", - "scipy", - "tornado", + "pytest", "typing_extensions", Review Comment:  The testing framework `pytest` is listed as a core runtime dependency in `dependencies`. Production users installing TVM do not need `pytest` to run inference or compile models. It should be moved to `[dependency-groups]` or `[project.optional-dependencies]` (e.g., under `test` or `dev`) to keep the production dependency graph minimal. ```suggestion "psutil", "typing_extensions", ``` ########## ci/scripts/package/windows_build_libtvm_runtime_cuda.sh: ########## @@ -0,0 +1,101 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +# Build tvm_runtime_cuda.dll on a Windows runner, run by the build_cuda_runtime CI +# job (on the host; unlike Linux there is no build container on Windows). Installs +# the pinned CUDA toolkit via conda and builds the sidecar into build-wheel-cuda/lib/ +# for the wheel build to bundle. Windows mirror of manylinux_build_libtvm_runtime_cuda.sh. +# +# Usage: windows_build_libtvm_runtime_cuda.sh +set -euxo pipefail + +# Keep a unix-style path for bash file operations and a mixed (forward-slash) +# path for CMake/cmd, which dislike the /c/... msys form. +repo_root_unix="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)" +repo_root="$(cygpath -m "${repo_root_unix}")" +build_dir_unix="${repo_root_unix}/build-wheel-cuda" +build_dir="$(cygpath -m "${build_dir_unix}")" +cuda_prefix_unix="/c/opt/cuda" +cuda_prefix="C:/opt/cuda" + +# Locate conda: this script runs under a non-login bash, so conda may not be on +# PATH even though the runner ships Miniconda (exposed via $CONDA). +conda_exe="$(command -v conda || true)" +if [[ -z "${conda_exe}" ]]; then + conda_exe="${CONDA:-/c/Miniconda}/Scripts/conda.exe" +fi + +# Install the pinned CUDA toolkit via conda from the nvidia channel, mirroring the +# LLVM-via-conda install used elsewhere in the publish action. The win-64 channel +# caps at 13.0.x, matching the Linux hook's CUDA 13.0.2. +if [[ ! -e "${cuda_prefix_unix}/Library/bin/nvcc.exe" ]]; then + "${conda_exe}" create -q -p "${cuda_prefix}" -c nvidia/label/cuda-13.0.2 cuda-toolkit -y \ + || "${conda_exe}" create -q -p "${cuda_prefix}" -c nvidia/label/cuda-13.0.2 cuda-toolkit --use-only-tar-bz2 -y Review Comment:  If the first `conda create` command fails halfway, the target directory `${cuda_prefix}` may be left partially populated. The fallback command will then fail because the directory is not empty. Cleaning up the directory before retrying ensures the fallback command can run successfully. ```suggestion "${conda_exe}" create -q -p "${cuda_prefix}" -c nvidia/label/cuda-13.0.2 cuda-toolkit -y \ || { rm -rf "${cuda_prefix_unix}" && "${conda_exe}" create -q -p "${cuda_prefix}" -c nvidia/label/cuda-13.0.2 cuda-toolkit --use-only-tar-bz2 -y; } ``` ########## ci/scripts/package/windows_build_libtvm_runtime_cuda.sh: ########## @@ -0,0 +1,101 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +# Build tvm_runtime_cuda.dll on a Windows runner, run by the build_cuda_runtime CI +# job (on the host; unlike Linux there is no build container on Windows). Installs +# the pinned CUDA toolkit via conda and builds the sidecar into build-wheel-cuda/lib/ +# for the wheel build to bundle. Windows mirror of manylinux_build_libtvm_runtime_cuda.sh. +# +# Usage: windows_build_libtvm_runtime_cuda.sh +set -euxo pipefail + +# Keep a unix-style path for bash file operations and a mixed (forward-slash) +# path for CMake/cmd, which dislike the /c/... msys form. +repo_root_unix="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)" +repo_root="$(cygpath -m "${repo_root_unix}")" +build_dir_unix="${repo_root_unix}/build-wheel-cuda" +build_dir="$(cygpath -m "${build_dir_unix}")" +cuda_prefix_unix="/c/opt/cuda" +cuda_prefix="C:/opt/cuda" + +# Locate conda: this script runs under a non-login bash, so conda may not be on +# PATH even though the runner ships Miniconda (exposed via $CONDA). +conda_exe="$(command -v conda || true)" +if [[ -z "${conda_exe}" ]]; then + conda_exe="${CONDA:-/c/Miniconda}/Scripts/conda.exe" +fi + +# Install the pinned CUDA toolkit via conda from the nvidia channel, mirroring the +# LLVM-via-conda install used elsewhere in the publish action. The win-64 channel +# caps at 13.0.x, matching the Linux hook's CUDA 13.0.2. +if [[ ! -e "${cuda_prefix_unix}/Library/bin/nvcc.exe" ]]; then + "${conda_exe}" create -q -p "${cuda_prefix}" -c nvidia/label/cuda-13.0.2 cuda-toolkit -y \ + || "${conda_exe}" create -q -p "${cuda_prefix}" -c nvidia/label/cuda-13.0.2 cuda-toolkit --use-only-tar-bz2 -y +fi + +# conda lays the Windows toolkit out under <prefix>/Library (bin/nvcc.exe, +# lib/x64/cudart.lib, include/...). Discover the root from nvcc.exe so TVM's +# FindCUDA MSVC branch resolves against the real layout instead of a hardcode. +nvcc_unix="$(find "${cuda_prefix_unix}" -iname nvcc.exe | head -n1)" +test -n "${nvcc_unix}" +nvcc_exe="$(cygpath -m "${nvcc_unix}")" +cuda_root="$(cygpath -m "$(dirname "$(dirname "${nvcc_unix}")")")" # <prefix>/Library +export CUDA_PATH="${cuda_root}" + +python -m pip install -U pip cmake ninja +"${nvcc_exe}" --version + +# nvcc needs the MSVC host compiler (cl.exe) on PATH, but this bash is not a VS +# Developer shell. Locate VS via vswhere and run the cmake configure+build inside +# vcvars64. +vswhere="C:/Program Files (x86)/Microsoft Visual Studio/Installer/vswhere.exe" +vs_path="$("${vswhere}" -latest -products '*' \ + -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 \ + -property installationPath | tr -d '\r')" +test -n "${vs_path}" +vcvars="${vs_path}\\VC\\Auxiliary\\Build\\vcvars64.bat" + +rm -rf "${build_dir_unix}" + +# CMAKE_CUDA_COMPILER only tells CMake which nvcc to use (load-bearing here: the +# conda nvcc is not on PATH); it does not affect the resulting tvm_runtime_cuda.dll, +# which is built only from .cc host sources (no .cu device code). CMAKE_CUDA_ARCHITECTURES +# is intentionally not set -- a no-op for the same reason, and modern CMake fills a +# default. -allow-unsupported-compiler guards against the runner's MSVC being newer +# than the CUDA toolkit officially supports. +cmd_script="$(mktemp --suffix=.bat)" +cat > "${cmd_script}" <<EOF +call "${vcvars}" || exit /b 1 +cmake -S "${repo_root}" -B "${build_dir}" -G Ninja ^ + -DCMAKE_BUILD_TYPE=Release ^ + -DBUILD_TESTING=OFF ^ + -DTVM_BUILD_PYTHON_MODULE=ON ^ + -DUSE_CUDA="${cuda_root}" ^ + -DUSE_LLVM=OFF ^ + -DUSE_CUBLAS=OFF -DUSE_CUDNN=OFF -DUSE_CUTLASS=OFF -DUSE_NCCL=OFF -DUSE_NVTX=OFF ^ + -DCMAKE_CUDA_COMPILER="${nvcc_exe}" ^ + -DCMAKE_CUDA_FLAGS="-allow-unsupported-compiler" || exit /b 1 Review Comment:  Using line continuations with `^` in a generated Windows batch file is fragile because any trailing whitespace after the `^` character will break the command. It is safer to write the entire `cmake` command on a single line to prevent accidental syntax errors during execution. ```suggestion cmake -S "${repo_root}" -B "${build_dir}" -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=OFF -DTVM_BUILD_PYTHON_MODULE=ON -DUSE_CUDA="${cuda_root}" -DUSE_LLVM=OFF -DUSE_CUBLAS=OFF -DUSE_CUDNN=OFF -DUSE_CUTLASS=OFF -DUSE_NCCL=OFF -DUSE_NVTX=OFF -DCMAKE_CUDA_COMPILER="${nvcc_exe}" -DCMAKE_CUDA_FLAGS="-allow-unsupported-compiler" || exit /b 1 ``` ########## CMakeLists.txt: ########## @@ -592,6 +584,25 @@ target_include_directories(tvm_compiler PUBLIC "$<INSTALL_INTERFACE:${CMAKE_INST set_property(TARGET tvm_compiler APPEND PROPERTY LINK_OPTIONS "${TVM_NO_UNDEFINED_SYMBOLS}") set_property(TARGET tvm_compiler APPEND PROPERTY LINK_OPTIONS "${TVM_VISIBILITY_FLAG}") +# Work around a GNU ld (binutils) relaxation bug that miscompiles +# R_X86_64_GOTPCRELX relocations inside very large statically-linked archives. +# When the full LLVM static libraries are linked into libtvm_compiler.so, the +# library is large enough that ld can relax an indirect GOT call (LLVM built +# with -fno-plt emits these) into a direct call with an incorrect displacement. +# The call then targets read-only data instead of the intended function and +# crashes at runtime with a SIGSEGV inside llvm::X86Subtarget during code +# generation. Disabling linker relaxation keeps the GOT-indirect sequences and +# avoids the miscompilation; it is harmless when LLVM is linked dynamically. +# See binutils bug ld/25754. +if(CMAKE_SYSTEM_NAME STREQUAL "Linux" AND NOT ${USE_LLVM} MATCHES ${IS_FALSE_PATTERN}) Review Comment:  In CMake, if `${USE_LLVM}` is set to a path (e.g., to a specific `llvm-config`), expanding it unquoted in the `if` condition can cause syntax errors if the path contains spaces or special characters. Quoting the variable as `"${USE_LLVM}"` is safer and prevents potential configuration failures. ``` if(CMAKE_SYSTEM_NAME STREQUAL "Linux" AND NOT "${USE_LLVM}" MATCHES ${IS_FALSE_PATTERN}) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
