gemini-code-assist[bot] commented on code in PR #225: URL: https://github.com/apache/tvm-ffi/pull/225#discussion_r2492312231
########## 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:  These repetitive calls to `build_libs` can be simplified by using a `for` loop. This makes the code more concise and easier to maintain when adding or removing torch versions. ```suggestion for version in "2.4" "2.5" "2.6" "2.7" "2.8" "2.9"; do build_libs "$version" done ``` ########## 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 `if/elif/else` chain can be refactored into a `case` statement for better readability and maintainability, especially since you are comparing the same variable against multiple string literals. This also makes it easier to group versions with 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 Review Comment:  It's good practice to validate script arguments. This prevents errors from unset variables when `set -u` is active and provides helpful usage information to the user if the script is called incorrectly. ```suggestion if [[ $# -ne 2 ]]; then echo "Usage: $0 <arch> <python_version>" >&2 exit 1 fi arch=$1 python_version=$2 ``` ########## 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 and avoid confusion. ########## 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:  This function can be made more readable by using a `case` statement instead of a long `if/elif` chain. Additionally, the logic `return $([[ ... ]] && echo 1 || echo 0)` is a bit obscure. A more conventional approach with explicit `return` statements would improve clarity and maintainability. ```shell function check_availability() { local torch_version="$1" case "$torch_version" in "2.4") if [[ "$arch" == "aarch64" || "$python_version" == "cp313" || "$python_version" == "cp314" ]]; then return 1 fi ;; "2.5" | "2.6") if [[ "$arch" == "aarch64" || "$python_version" == "cp314" ]]; then return 1 fi ;; "2.7" | "2.8") if [[ "$python_version" == "cp314" ]]; then return 1 fi ;; "2.9") if [[ "$python_version" == "cp39" ]]; then return 1 fi ;; *) echo "Unknown or unsupported torch version: $torch_version" >&2 return 1 ;; esac return 0 } ``` -- 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]
