gemini-code-assist[bot] commented on code in PR #19953: URL: https://github.com/apache/tvm/pull/19953#discussion_r3525136086
########## tests/python/disco/test_custom_allreduce.py: ########## @@ -26,7 +26,6 @@ import tvm import tvm.testing from tvm.runtime import DataType, disco -from tvm.runtime.disco import Session class AllReduceStrategyType(enum.IntEnum): Review Comment:  If the `disco` runtime is not enabled/compiled in the TVM build, `disco` will be imported as `None`. To prevent subsequent import-time attribute accesses or other errors, we should add a module-level skip check right after the imports, similar to how it is done in `test_ccl.py`. ```suggestion from tvm.runtime import DataType, disco if disco is None: pytest.skip("disco runtime is not available", allow_module_level=True) class AllReduceStrategyType(enum.IntEnum): ``` ########## tests/python/conftest.py: ########## @@ -14,17 +14,55 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""Configure pytest""" +"""Configure pytest for TVM's Python test suite.""" -import sys +import os -COLLECT_IGNORE = [] -if sys.platform.startswith("win"): - COLLECT_IGNORE.append("frontend/coreml") - COLLECT_IGNORE.append("frontend/keras") - COLLECT_IGNORE.append("frontend/pytorch") - COLLECT_IGNORE.append("frontend/tensorflow") - COLLECT_IGNORE.append("frontend/tflite") - COLLECT_IGNORE.append("frontend/onnx") +import _pytest - COLLECT_IGNORE.append("tir_base/test_tir_intrin.py") + +def pytest_collection_modifyitems(items): + """Maintain the ordering and cache bookkeeping required by TVM fixtures.""" + _count_num_fixture_uses(items) + _remove_global_fixture_definitions(items) + _sort_tests(items) + + +def _count_num_fixture_uses(items): + for item in items: + is_skipped = item.get_closest_marker("skip") or any( + mark.args[0] for mark in item.iter_markers("skipif") + ) + if is_skipped: + continue + + for fixturedefs in item._fixtureinfo.name2fixturedefs.values(): + fixturedef = fixturedefs[-1] + if hasattr(fixturedef.func, "num_tests_use_this_fixture"): + fixturedef.func.num_tests_use_this_fixture[0] += 1 + + +def _remove_global_fixture_definitions(items): + modules = {item.module for item in items} + for module in modules: + for name in dir(module): + obj = getattr(module, name) + if hasattr(obj, "_pytestfixturefunction") and isinstance( + obj._pytestfixturefunction, _pytest.fixtures.FixtureFunctionMarker + ): + delattr(module, name) + + +def _sort_tests(items): + def sort_key(item): + filename, lineno, test_name = item.location + return filename, lineno, test_name.split("[")[0] + + items.sort(key=sort_key) + + +def pytest_sessionstart(): + if os.getenv("CI", "") == "true": + from request_hook import init # pylint: disable=import-outside-toplevel + + init() Review Comment:  Depending on how pytest is invoked (e.g., from different working directories or with different import modes), the `tests/python` directory might not be present in `sys.path`. To prevent a potential `ModuleNotFoundError` when importing `request_hook` in CI, we should explicitly add the directory of `conftest.py` to `sys.path` before importing. ```suggestion def pytest_sessionstart(): if os.getenv("CI", "") == "true": import sys from pathlib import Path sys.path.insert(0, str(Path(__file__).parent)) from request_hook import init # pylint: disable=import-outside-toplevel init() ``` ########## tests/python/conftest.py: ########## @@ -14,17 +14,55 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""Configure pytest""" +"""Configure pytest for TVM's Python test suite.""" -import sys +import os -COLLECT_IGNORE = [] -if sys.platform.startswith("win"): - COLLECT_IGNORE.append("frontend/coreml") - COLLECT_IGNORE.append("frontend/keras") - COLLECT_IGNORE.append("frontend/pytorch") - COLLECT_IGNORE.append("frontend/tensorflow") - COLLECT_IGNORE.append("frontend/tflite") - COLLECT_IGNORE.append("frontend/onnx") +import _pytest - COLLECT_IGNORE.append("tir_base/test_tir_intrin.py") + +def pytest_collection_modifyitems(items): + """Maintain the ordering and cache bookkeeping required by TVM fixtures.""" + _count_num_fixture_uses(items) + _remove_global_fixture_definitions(items) + _sort_tests(items) + + +def _count_num_fixture_uses(items): + for item in items: + is_skipped = item.get_closest_marker("skip") or any( + mark.args[0] for mark in item.iter_markers("skipif") + ) + if is_skipped: + continue + + for fixturedefs in item._fixtureinfo.name2fixturedefs.values(): + fixturedef = fixturedefs[-1] + if hasattr(fixturedef.func, "num_tests_use_this_fixture"): + fixturedef.func.num_tests_use_this_fixture[0] += 1 Review Comment:  To ensure robust defensive programming, we should verify that `item` actually has the `_fixtureinfo` attribute before accessing it. Non-standard or custom pytest items might not have this attribute, which would otherwise raise an `AttributeError`. ```suggestion def _count_num_fixture_uses(items): for item in items: is_skipped = item.get_closest_marker("skip") or any( mark.args[0] for mark in item.iter_markers("skipif") ) if is_skipped: continue if not hasattr(item, "_fixtureinfo") or item._fixtureinfo is None: continue for fixturedefs in item._fixtureinfo.name2fixturedefs.values(): fixturedef = fixturedefs[-1] if hasattr(fixturedef.func, "num_tests_use_this_fixture"): fixturedef.func.num_tests_use_this_fixture[0] += 1 ``` ########## tests/python/disco/test_custom_allreduce.py: ########## @@ -56,7 +55,7 @@ class AllReduceStrategyType(enum.IntEnum): @pytest.mark.parametrize("strategy", _strategies) def test_allreduce(shape, ccl, strategy): devices = [0, 1] - sess: Session = disco.ProcessSession(num_workers=len(devices)) + sess: disco.Session = disco.ProcessSession(num_workers=len(devices)) Review Comment:  The type annotation `sess: disco.Session` is evaluated at import time in Python 3.10 (since `from __future__ import annotations` is not used). If `disco` is `None`, this will raise an `AttributeError` during module import. Removing the type annotation entirely is cleaner and avoids any import-time issues. ```suggestion sess = disco.ProcessSession(num_workers=len(devices)) ``` ########## tests/python/tirx/conftest.py: ########## @@ -33,8 +35,10 @@ def pytest_collection_modifyitems(config, items): if env.has_cuda_compute(10): return + suite_root = Path(__file__).parent skip = pytest.mark.skip( reason="tirx suite requires a CUDA compute capability 10.0 (sm_100a) device" ) for item in items: - item.add_marker(skip) + if item.path.is_relative_to(suite_root): + item.add_marker(skip) Review Comment:  To ensure robust defensive programming, we should check if `item` has the `path` attribute before accessing it. Custom or non-file pytest items might not have a `path` attribute, which would otherwise raise an `AttributeError`. ```suggestion for item in items: path = getattr(item, "path", None) if path and path.is_relative_to(suite_root): item.add_marker(skip) ``` ########## tests/python/conftest.py: ########## @@ -14,17 +14,55 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""Configure pytest""" +"""Configure pytest for TVM's Python test suite.""" -import sys +import os -COLLECT_IGNORE = [] -if sys.platform.startswith("win"): - COLLECT_IGNORE.append("frontend/coreml") - COLLECT_IGNORE.append("frontend/keras") - COLLECT_IGNORE.append("frontend/pytorch") - COLLECT_IGNORE.append("frontend/tensorflow") - COLLECT_IGNORE.append("frontend/tflite") - COLLECT_IGNORE.append("frontend/onnx") +import _pytest - COLLECT_IGNORE.append("tir_base/test_tir_intrin.py") + +def pytest_collection_modifyitems(items): + """Maintain the ordering and cache bookkeeping required by TVM fixtures.""" + _count_num_fixture_uses(items) + _remove_global_fixture_definitions(items) + _sort_tests(items) + + +def _count_num_fixture_uses(items): + for item in items: + is_skipped = item.get_closest_marker("skip") or any( + mark.args[0] for mark in item.iter_markers("skipif") + ) + if is_skipped: + continue + + for fixturedefs in item._fixtureinfo.name2fixturedefs.values(): + fixturedef = fixturedefs[-1] + if hasattr(fixturedef.func, "num_tests_use_this_fixture"): + fixturedef.func.num_tests_use_this_fixture[0] += 1 + + +def _remove_global_fixture_definitions(items): + modules = {item.module for item in items} + for module in modules: + for name in dir(module): + obj = getattr(module, name) + if hasattr(obj, "_pytestfixturefunction") and isinstance( + obj._pytestfixturefunction, _pytest.fixtures.FixtureFunctionMarker + ): + delattr(module, name) Review Comment:  To ensure robust defensive programming, we should verify that `item` has the `module` attribute and that it is not `None` before attempting to inspect its directory. Non-python or custom pytest items might not have a `module` attribute. ```suggestion def _remove_global_fixture_definitions(items): modules = {item.module for item in items if hasattr(item, "module") and item.module is not None} for module in modules: for name in dir(module): obj = getattr(module, name) if hasattr(obj, "_pytestfixturefunction") and isinstance( obj._pytestfixturefunction, _pytest.fixtures.FixtureFunctionMarker ): delattr(module, name) ``` -- 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]
