gemini-code-assist[bot] commented on code in PR #19707:
URL: https://github.com/apache/tvm/pull/19707#discussion_r3384428866
##########
python/tvm/libinfo.py:
##########
@@ -259,42 +259,62 @@ def find_include_path(name=None, search_path=None,
optional=False):
include_path : list(string)
List of all found paths to header files.
"""
+ def _as_list(value):
+ if value is None:
+ return []
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return [value]
+
+ def _append_unique(values, value):
+ value = os.path.abspath(value)
+ if value not in values:
+ values.append(value)
+
+ def _append_existing_dir(values, value):
+ value = os.path.abspath(value)
+ if os.path.exists(value) and os.path.isdir(value) and value not in
values:
+ values.append(value)
Review Comment:
In `_append_unique` and `_append_existing_dir`, `os.path.abspath(value)` is
called. However, `include_candidates` is already converted to absolute paths
using `os.path.abspath` at line 334 before these helpers are called. Therefore,
calling `os.path.abspath` inside these helpers is redundant.
Additionally, in `_append_existing_dir`, `os.path.isdir(value)` implicitly
checks if the path exists, making the `os.path.exists(value)` check redundant.
We can simplify these helper functions to improve readability and
performance.
```suggestion
def _append_unique(values, value):
if value not in values:
values.append(value)
def _append_existing_dir(values, value):
if os.path.isdir(value) and value not in values:
values.append(value)
```
##########
python/tvm/libinfo.py:
##########
@@ -259,42 +259,62 @@ def find_include_path(name=None, search_path=None,
optional=False):
include_path : list(string)
List of all found paths to header files.
"""
+ def _as_list(value):
+ if value is None:
+ return []
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return [value]
+
+ def _append_unique(values, value):
+ value = os.path.abspath(value)
+ if value not in values:
+ values.append(value)
+
+ def _append_existing_dir(values, value):
+ value = os.path.abspath(value)
+ if os.path.exists(value) and os.path.isdir(value) and value not in
values:
+ values.append(value)
+
+ ffi_dir = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
+ source_dirs = []
if os.environ.get("TVM_SOURCE_DIR", None):
- source_dir = os.environ["TVM_SOURCE_DIR"]
+ source_dirs.append(os.environ["TVM_SOURCE_DIR"])
elif os.environ.get("TVM_HOME", None):
- source_dir = os.environ["TVM_HOME"]
+ source_dirs.append(os.environ["TVM_HOME"])
else:
- ffi_dir =
os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
- for source_dir in ["..", "../..", "../../.."]:
- source_dir = os.path.join(ffi_dir, source_dir)
- if os.path.isdir(os.path.join(source_dir, "include")):
- break
- else:
- raise AssertionError(f"Cannot find the source directory given
ffi_dir: {ffi_dir}")
- third_party_dir = os.path.join(source_dir, "3rdparty")
+ for relpath in ["", "..", "../..", "../../.."]:
+ candidate = os.path.join(ffi_dir, relpath)
+ if os.path.isdir(os.path.join(candidate, "include")):
+ source_dirs.append(candidate)
header_path = []
if os.environ.get("TVM_INCLUDE_PATH", None):
header_path.append(os.environ["TVM_INCLUDE_PATH"])
- header_path.append(source_dir)
- header_path.append(third_party_dir)
+ for source_dir in source_dirs:
+ header_path.append(source_dir)
+ header_path.append(os.path.join(source_dir, "3rdparty"))
header_path = [os.path.abspath(x) for x in header_path]
if search_path is not None:
- if isinstance(search_path, list):
- header_path = header_path + search_path
- else:
- header_path.append(search_path)
+ header_path = header_path + _as_list(search_path)
+
+ tvm_ffi_package_include_path = []
+ dlpack_include_path = []
+ try:
+ from tvm_ffi import libinfo as tvm_ffi_libinfo # pylint:
disable=import-outside-toplevel
+
+ tvm_ffi_package_include_path += tvm_ffi_libinfo.include_paths()
+ except (ImportError, RuntimeError):
+ pass
Review Comment:
In the `try-except` block, only `ImportError` and `RuntimeError` are caught
when importing and calling `tvm_ffi_libinfo.include_paths()`. If `tvm_ffi` is
present but doesn't have the `libinfo` module or the `include_paths` attribute
(e.g., due to an older version or a partial installation), an `AttributeError`
will be raised, which is not caught and could crash the execution.
We should add `AttributeError` to the caught exceptions to make this import
and fallback more robust.
```suggestion
try:
from tvm_ffi import libinfo as tvm_ffi_libinfo # pylint:
disable=import-outside-toplevel
tvm_ffi_package_include_path += tvm_ffi_libinfo.include_paths()
except (ImportError, RuntimeError, AttributeError):
pass
```
##########
python/tvm/libinfo.py:
##########
@@ -259,42 +259,62 @@ def find_include_path(name=None, search_path=None,
optional=False):
include_path : list(string)
List of all found paths to header files.
"""
+ def _as_list(value):
+ if value is None:
+ return []
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return [value]
+
+ def _append_unique(values, value):
+ value = os.path.abspath(value)
+ if value not in values:
+ values.append(value)
+
+ def _append_existing_dir(values, value):
+ value = os.path.abspath(value)
+ if os.path.exists(value) and os.path.isdir(value) and value not in
values:
+ values.append(value)
+
+ ffi_dir = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
+ source_dirs = []
if os.environ.get("TVM_SOURCE_DIR", None):
- source_dir = os.environ["TVM_SOURCE_DIR"]
+ source_dirs.append(os.environ["TVM_SOURCE_DIR"])
elif os.environ.get("TVM_HOME", None):
- source_dir = os.environ["TVM_HOME"]
+ source_dirs.append(os.environ["TVM_HOME"])
else:
- ffi_dir =
os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
- for source_dir in ["..", "../..", "../../.."]:
- source_dir = os.path.join(ffi_dir, source_dir)
- if os.path.isdir(os.path.join(source_dir, "include")):
- break
- else:
- raise AssertionError(f"Cannot find the source directory given
ffi_dir: {ffi_dir}")
- third_party_dir = os.path.join(source_dir, "3rdparty")
+ for relpath in ["", "..", "../..", "../../.."]:
+ candidate = os.path.join(ffi_dir, relpath)
+ if os.path.isdir(os.path.join(candidate, "include")):
+ source_dirs.append(candidate)
header_path = []
if os.environ.get("TVM_INCLUDE_PATH", None):
header_path.append(os.environ["TVM_INCLUDE_PATH"])
- header_path.append(source_dir)
- header_path.append(third_party_dir)
+ for source_dir in source_dirs:
+ header_path.append(source_dir)
+ header_path.append(os.path.join(source_dir, "3rdparty"))
header_path = [os.path.abspath(x) for x in header_path]
if search_path is not None:
- if isinstance(search_path, list):
- header_path = header_path + search_path
- else:
- header_path.append(search_path)
+ header_path = header_path + _as_list(search_path)
+
+ tvm_ffi_package_include_path = []
+ dlpack_include_path = []
Review Comment:
The variable `dlpack_include_path` is initialized to `[]` at line 305, but
it is only used in the `else` block (starting at line 318) where it is
immediately overwritten. It is not used in the `if name is not None:` block or
anywhere else. This initialization is redundant and can be safely removed.
```suggestion
tvm_ffi_package_include_path = []
```
--
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]
