Copilot commented on code in PR #847:
URL: https://github.com/apache/mahout/pull/847#discussion_r2701177293
##########
testing/conftest.py:
##########
@@ -13,20 +13,95 @@
# 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.
-#
-"""Shared pytest configuration and fixtures for all tests."""
+"""
+Shared pytest configuration and fixtures for all tests.
-# Skip qdp tests if dependencies are not available
-collect_ignore_glob = []
+This module provides:
+- Custom pytest markers (gpu, slow)
+- Auto-skip logic for QDP tests when the native extension is not built
+- Shared fixtures for QDP availability checking
+
+QDP tests are automatically skipped if the _qdp extension is not available,
+allowing contributors without CUDA to run the qumat test suite.
+"""
+import pytest
+
+# Check if QDP extension is available at module load time
+_QDP_AVAILABLE = False
+_QDP_IMPORT_ERROR: str | None = "No module named '_qdp'"
Review Comment:
The initial assignment of _QDP_IMPORT_ERROR to "No module named '_qdp'" on
line 33 is redundant, as this value will always be overwritten either to None
(line 38) if import succeeds or to str(e) (line 40) if import fails. Consider
removing this initial assignment for clarity.
```suggestion
_QDP_IMPORT_ERROR: str | None
```
##########
testing/conftest.py:
##########
@@ -13,20 +13,95 @@
# 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.
-#
-"""Shared pytest configuration and fixtures for all tests."""
+"""
+Shared pytest configuration and fixtures for all tests.
-# Skip qdp tests if dependencies are not available
-collect_ignore_glob = []
+This module provides:
+- Custom pytest markers (gpu, slow)
+- Auto-skip logic for QDP tests when the native extension is not built
+- Shared fixtures for QDP availability checking
+
+QDP tests are automatically skipped if the _qdp extension is not available,
+allowing contributors without CUDA to run the qumat test suite.
+"""
+import pytest
+
+# Check if QDP extension is available at module load time
+_QDP_AVAILABLE = False
+_QDP_IMPORT_ERROR: str | None = "No module named '_qdp'"
try:
- import _qdp # noqa: F401
- import torch # noqa: F401
-except ImportError:
+ import _qdp # noqa: F401, PLC0415
Review Comment:
Import of '_qdp' is not used.
```suggestion
import _qdp # noqa: F401, PLC0415
del _qdp
```
##########
testing/conftest.py:
##########
@@ -13,20 +13,95 @@
# 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.
-#
-"""Shared pytest configuration and fixtures for all tests."""
+"""
+Shared pytest configuration and fixtures for all tests.
-# Skip qdp tests if dependencies are not available
-collect_ignore_glob = []
+This module provides:
+- Custom pytest markers (gpu, slow)
+- Auto-skip logic for QDP tests when the native extension is not built
+- Shared fixtures for QDP availability checking
+
+QDP tests are automatically skipped if the _qdp extension is not available,
+allowing contributors without CUDA to run the qumat test suite.
+"""
+import pytest
+
+# Check if QDP extension is available at module load time
+_QDP_AVAILABLE = False
+_QDP_IMPORT_ERROR: str | None = "No module named '_qdp'"
try:
- import _qdp # noqa: F401
- import torch # noqa: F401
-except ImportError:
+ import _qdp # noqa: F401, PLC0415
+
+ _QDP_AVAILABLE = True
+ _QDP_IMPORT_ERROR = None
+except ImportError as e:
+ _QDP_IMPORT_ERROR = str(e)
Review Comment:
The QDP availability check has changed from using importlib.util.find_spec
to a try/except import. While both approaches work, the try/except approach
will catch all import errors (including those from broken installations or
dependency issues), whereas find_spec only checks if the module can be located.
This behavioral change could make debugging more difficult if _qdp is found but
fails to import due to dependency issues, as the error message will be more
generic. Consider whether this change in behavior is intentional.
--
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]
