Repository navigation
BUG: Show warning for ignored CPU feature names - #33027
mikamikasuki wants to merge 2 commits into
Conversation
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
It looks like @seiko2plus intentionally suggested this (although visibility also came up) in #22137 (comment). Maybe he has an opinion here? That said, I think it does make sense to make a visible warning about this.
I think it also probably needs a release note.
| @pytest.mark.skipif( | ||
| not __cpu_dispatch__ or "AVX512F" in __cpu_dispatch__, | ||
| reason="AVX512F is still a dispatched feature in this build", | ||
| ) | ||
| def test_legacy_feature_name_warning_is_visible(self): | ||
| """Legacy feature names that are no longer dispatched are visible.""" | ||
| self.env['NPY_DISABLE_CPU_FEATURES'] = "AVX512F" | ||
| out = self._run() | ||
| assert "UserWarning" in out.stderr | ||
| assert "AVX512F" in out.stderr |
There was a problem hiding this comment.
Could we use a nonexistent feature name and cover both environment variables? Something like this (I used an AI model):
| @pytest.mark.skipif( | |
| not __cpu_dispatch__ or "AVX512F" in __cpu_dispatch__, | |
| reason="AVX512F is still a dispatched feature in this build", | |
| ) | |
| def test_legacy_feature_name_warning_is_visible(self): | |
| """Legacy feature names that are no longer dispatched are visible.""" | |
| self.env['NPY_DISABLE_CPU_FEATURES'] = "AVX512F" | |
| out = self._run() | |
| assert "UserWarning" in out.stderr | |
| assert "AVX512F" in out.stderr | |
| @pytest.mark.skipif( | |
| not __cpu_dispatch__, | |
| reason=( | |
| "NPY_*_CPU_FEATURES only parsed if " | |
| "`__cpu_dispatch__` is non-empty" | |
| ) | |
| ) | |
| @pytest.mark.parametrize("action", ["ENABLE", "DISABLE"]) | |
| def test_unknown_feature_warning_is_visible(self, action): | |
| feature = "NOT_A_CPU_FEATURE" | |
| self.env[f"NPY_{action}_CPU_FEATURES"] = feature | |
| self.env.pop("PYTHONWARNINGS", None) | |
| out = self._run() | |
| assert re.search( | |
| rf"UserWarning:.*{re.escape(feature)}", | |
| out.stderr, | |
| flags=re.DOTALL, | |
| ) |
seiko2plus
left a comment
seiko2plus
left a comment
There was a problem hiding this comment.
At first sight (#33016 (comment)), I didn't like the idea but after re-considering yes indeed it should be treated as UserWarning. However, you missed a case here that needs to be changed too:
numpy/numpy/_core/src/common/npy_cpu_features.c
Lines 374 to 377 in 6a65c3c
PR summary
Fixes #33016.
Previously, when NPY_DISABLE_CPU_FEATURES contained an unavailable or unrecognized CPU feature name, NumPy emitted an ImportWarning, which is normally hidden by Python's default warning filters. As a result, users could mistakenly believe that their requested CPU feature configuration had been applied.
This PR changes the warning category to UserWarning, making the ignored feature names visible under Python's default warning settings. The change is limited to warning visibility and does not alter CPU feature detection or dispatch behavior.
Validation: 12 CPU feature tests passed, with 6 platform-specific tests skipped.
First time contributor introduction
Hi! I'm a computer science student interested in scientific computing and open-source software. I use NumPy in machine learning and research projects, and I enjoy investigating small issues that can make numerical software more reliable and easier to use.
AI disclosure
Parts of this change were developed with AI assistance. I reviewed the changes and validated the behavior with the relevant tests.