gemini-code-assist[bot] commented on code in PR #225: URL: https://github.com/apache/tvm-ffi/pull/225#discussion_r2492314053
########## addons/torch_c_dlpack_ext/build_aot_libs.sh: ########## @@ -0,0 +1,92 @@ +#!/bin/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. +set -eux + +arch=$1 +python_version=$2 + +tvm_ffi="$PWD" +torch_c_dlpack_ext="$tvm_ffi"/addons/torch_c_dlpack_ext + + +function get_torch_url() { + local version="$1" + if [[ "$version" == "2.4" ]]; then + echo "https://download.pytorch.org/whl/cu124"; + elif [[ "$version" == "2.5" ]]; then + echo "https://download.pytorch.org/whl/cu124"; + elif [[ "$version" == "2.6" ]]; then + echo "https://download.pytorch.org/whl/cu126"; + elif [[ "$version" == "2.7" ]]; then + echo "https://download.pytorch.org/whl/cu128"; + elif [[ "$version" == "2.8" ]]; then + echo "https://download.pytorch.org/whl/cu129"; + elif [[ "$version" == "2.9" ]]; then + echo "https://download.pytorch.org/whl/cu129"; + else + echo "Unknown or unsupported torch version: $version" >&2 + return 1 + fi +} + + +function check_availability() { + local torch_version="$1" + if [[ "$torch_version" == "2.4" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp313" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.5" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.6" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.7" ]]; then + return $([[ "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.8" ]]; then + return $([[ "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.9" ]]; then + return $([[ "$python_version" == "cp39" ]] && echo 1 || echo 0) + else + echo "Unknown or unsupported torch version: $torch_version" >&2 + return 1 + fi +} + + +function build_libs() { + local torch_version=$1 + if check_availability "$torch_version"; then + mkdir "$tvm_ffi"/.venv -p + uv venv "$tvm_ffi"/.venv/torch"$torch_version" --python $python_version + source "$tvm_ffi"/.venv/torch"$torch_version"/bin/activate + uv pip install setuptools ninja + uv pip install torch=="$torch_version" --index-url $(get_torch_url "$torch_version") + uv pip install -v . + mkdir "$tvm_ffi"/lib -p + python -m tvm_ffi.utils._build_optional_torch_c_dlpack --output-dir "$tvm_ffi"/lib + python -m tvm_ffi.utils._build_optional_torch_c_dlpack --output-dir "$tvm_ffi"/lib --build-with-cuda + ls "$tvm_ffi"/lib + deactivate + rm -rf "$tvm_ffi"/.venv/torch"$torch_version" + fi Review Comment:  When `check_availability` determines that a build should be skipped, the script silently does nothing. This can be confusing for someone running the script manually. It would be better to print a message indicating that the build for a specific version is being skipped. ```suggestion if check_availability "$torch_version"; then mkdir "$tvm_ffi"/.venv -p uv venv "$tvm_ffi"/.venv/torch"$torch_version" --python $python_version source "$tvm_ffi"/.venv/torch"$torch_version"/bin/activate uv pip install setuptools ninja uv pip install torch=="$torch_version" --index-url $(get_torch_url "$torch_version") uv pip install -v . mkdir "$tvm_ffi"/lib -p python -m tvm_ffi.utils._build_optional_torch_c_dlpack --output-dir "$tvm_ffi"/lib python -m tvm_ffi.utils._build_optional_torch_c_dlpack --output-dir "$tvm_ffi"/lib --build-with-cuda ls "$tvm_ffi"/lib deactivate rm -rf "$tvm_ffi"/.venv/torch"$torch_version" else echo "Skipping build for torch $torch_version on $arch with python $python_version as it is not available." fi ``` ########## addons/torch_c_dlpack_ext/build_aot_libs.sh: ########## @@ -0,0 +1,92 @@ +#!/bin/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. +set -eux + +arch=$1 +python_version=$2 + +tvm_ffi="$PWD" +torch_c_dlpack_ext="$tvm_ffi"/addons/torch_c_dlpack_ext + + +function get_torch_url() { + local version="$1" + if [[ "$version" == "2.4" ]]; then + echo "https://download.pytorch.org/whl/cu124"; + elif [[ "$version" == "2.5" ]]; then + echo "https://download.pytorch.org/whl/cu124"; + elif [[ "$version" == "2.6" ]]; then + echo "https://download.pytorch.org/whl/cu126"; + elif [[ "$version" == "2.7" ]]; then + echo "https://download.pytorch.org/whl/cu128"; + elif [[ "$version" == "2.8" ]]; then + echo "https://download.pytorch.org/whl/cu129"; + elif [[ "$version" == "2.9" ]]; then + echo "https://download.pytorch.org/whl/cu129"; + else + echo "Unknown or unsupported torch version: $version" >&2 + return 1 + fi +} + + +function check_availability() { + local torch_version="$1" + if [[ "$torch_version" == "2.4" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp313" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.5" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.6" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.7" ]]; then + return $([[ "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.8" ]]; then + return $([[ "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.9" ]]; then + return $([[ "$python_version" == "cp39" ]] && echo 1 || echo 0) + else + echo "Unknown or unsupported torch version: $torch_version" >&2 + return 1 + fi +} Review Comment:  The `check_availability` function uses a long `if/elif/else` chain and a complex `return $([[...]] && echo 1 || echo 0)` construct. This can be simplified for better readability and maintainability. Using a `case` statement would make the logic clearer. Also, shell functions can return the exit status of the last command, so the `return $(...)` can be replaced by a more idiomatic test expression. The function should return 0 (success) if the combination is available, and non-zero otherwise. The expression `! [[ <unavailability_condition> ]]` achieves this concisely. ```shell function check_availability() { local torch_version="$1" case "$torch_version" in "2.4") ! [[ "$arch" == "aarch64" || "$python_version" == "cp313" || "$python_version" == "cp314" ]] ;; "2.5" | "2.6") ! [[ "$arch" == "aarch64" || "$python_version" == "cp314" ]] ;; "2.7" | "2.8") ! [[ "$python_version" == "cp314" ]] ;; "2.9") ! [[ "$python_version" == "cp39" ]] ;; *) echo "Unknown or unsupported torch version: $torch_version" >&2 return 1 ;; esac } ``` ########## addons/torch_c_dlpack_ext/build_aot_libs.sh: ########## @@ -0,0 +1,92 @@ +#!/bin/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. +set -eux + +arch=$1 +python_version=$2 + +tvm_ffi="$PWD" +torch_c_dlpack_ext="$tvm_ffi"/addons/torch_c_dlpack_ext Review Comment:  The variable `torch_c_dlpack_ext` is defined but never used in the script. It should be removed to improve code clarity. ########## addons/torch_c_dlpack_ext/build_aot_libs.sh: ########## @@ -0,0 +1,92 @@ +#!/bin/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. +set -eux + +arch=$1 +python_version=$2 + +tvm_ffi="$PWD" +torch_c_dlpack_ext="$tvm_ffi"/addons/torch_c_dlpack_ext + + +function get_torch_url() { + local version="$1" + if [[ "$version" == "2.4" ]]; then + echo "https://download.pytorch.org/whl/cu124"; + elif [[ "$version" == "2.5" ]]; then + echo "https://download.pytorch.org/whl/cu124"; + elif [[ "$version" == "2.6" ]]; then + echo "https://download.pytorch.org/whl/cu126"; + elif [[ "$version" == "2.7" ]]; then + echo "https://download.pytorch.org/whl/cu128"; + elif [[ "$version" == "2.8" ]]; then + echo "https://download.pytorch.org/whl/cu129"; + elif [[ "$version" == "2.9" ]]; then + echo "https://download.pytorch.org/whl/cu129"; + else + echo "Unknown or unsupported torch version: $version" >&2 + return 1 + fi +} Review Comment:  The `get_torch_url` function uses a long `if/elif/else` chain. This can be made more readable and maintainable by using a `case` statement, especially since some versions share the same URL. ```shell function get_torch_url() { local version="$1" case "$version" in "2.4" | "2.5") echo "https://download.pytorch.org/whl/cu124"; ;; "2.6") echo "https://download.pytorch.org/whl/cu126"; ;; "2.7") echo "https://download.pytorch.org/whl/cu128"; ;; "2.8" | "2.9") echo "https://download.pytorch.org/whl/cu129"; ;; *) echo "Unknown or unsupported torch version: $version" >&2 return 1 ;; esac } ``` ########## addons/torch_c_dlpack_ext/build_aot_libs.sh: ########## @@ -0,0 +1,92 @@ +#!/bin/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. +set -eux + +arch=$1 +python_version=$2 + +tvm_ffi="$PWD" +torch_c_dlpack_ext="$tvm_ffi"/addons/torch_c_dlpack_ext + + +function get_torch_url() { + local version="$1" + if [[ "$version" == "2.4" ]]; then + echo "https://download.pytorch.org/whl/cu124"; + elif [[ "$version" == "2.5" ]]; then + echo "https://download.pytorch.org/whl/cu124"; + elif [[ "$version" == "2.6" ]]; then + echo "https://download.pytorch.org/whl/cu126"; + elif [[ "$version" == "2.7" ]]; then + echo "https://download.pytorch.org/whl/cu128"; + elif [[ "$version" == "2.8" ]]; then + echo "https://download.pytorch.org/whl/cu129"; + elif [[ "$version" == "2.9" ]]; then + echo "https://download.pytorch.org/whl/cu129"; + else + echo "Unknown or unsupported torch version: $version" >&2 + return 1 + fi +} + + +function check_availability() { + local torch_version="$1" + if [[ "$torch_version" == "2.4" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp313" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.5" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.6" ]]; then + return $([[ "$arch" == "aarch64" || "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.7" ]]; then + return $([[ "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.8" ]]; then + return $([[ "$python_version" == "cp314" ]] && echo 1 || echo 0) + elif [[ "$torch_version" == "2.9" ]]; then + return $([[ "$python_version" == "cp39" ]] && echo 1 || echo 0) + else + echo "Unknown or unsupported torch version: $torch_version" >&2 + return 1 + fi +} + + +function build_libs() { + local torch_version=$1 + if check_availability "$torch_version"; then + mkdir "$tvm_ffi"/.venv -p + uv venv "$tvm_ffi"/.venv/torch"$torch_version" --python $python_version + source "$tvm_ffi"/.venv/torch"$torch_version"/bin/activate + uv pip install setuptools ninja + uv pip install torch=="$torch_version" --index-url $(get_torch_url "$torch_version") + uv pip install -v . + mkdir "$tvm_ffi"/lib -p + python -m tvm_ffi.utils._build_optional_torch_c_dlpack --output-dir "$tvm_ffi"/lib + python -m tvm_ffi.utils._build_optional_torch_c_dlpack --output-dir "$tvm_ffi"/lib --build-with-cuda + ls "$tvm_ffi"/lib + deactivate + rm -rf "$tvm_ffi"/.venv/torch"$torch_version" + fi +} + +build_libs "2.4" +build_libs "2.5" +build_libs "2.6" +build_libs "2.7" +build_libs "2.8" +build_libs "2.9" Review Comment:  The script calls `build_libs` for several hardcoded torch versions. This makes it harder to add or remove versions. It would be more maintainable to define the versions in an array and loop over them. ```suggestion torch_versions=("2.4" "2.5" "2.6" "2.7" "2.8" "2.9") for version in "${torch_versions[@]}"; do build_libs "$version" done ``` -- 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]
