Fail when a reason for not probing something stops being true - #19
Merged
Conversation
An entry named in UNPROBEABLE is unexamined by construction, so the reason has to keep being true. mv.elec.transport was excluded because BoltzTraP2 was "absent in this environment". Installing IFermi pulled BoltzTraP2 in as a dependency, the reason quietly stopped being true, and nothing noticed - which is how a tutorial calling that same function with the bands-axis object where a list of BandStructures belongs survived unseen, its ImportError swallowed by the cell's except. The new test fails when a reason claims a package is missing and that package imports. Verified by writing a stale reason deliberately and watching it fail, then reverting. The transport reason is rewritten to say what is actually true: the package is importable, its top level touches no netCDF, and BoltzTraP2.dft - which the transport path reaches - raises "numpy.dtype size changed" because netcdf4 was built against a different numpy. Still a valid exclusion, for a different reason than the one recorded. This came out of a scan for the pattern rather than from stumbling on it again. Two other sweeps found nothing: an AST pass over all 53 import guards in the package turned up only two functions importing something outside their guard, and both are deliberate fallbacks (requests to urllib, matplotlib_inline to IPython); and no probe entry reports "backend not installed" for a backend that is in fact installed. The stale-reason case was the only real instance. 972 tests pass on py3.12, 852 on py3.10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wh5NzNE9Wn49nuUm7zg2a
The guard added in the previous commit did its job immediately, and on CI rather than here: mv.mol.functional_groups was excluded on the reason that "neither openbabel nor libXrender is present in this environment", and CI installs both. The reason had been false there the whole time, and because an excluded entry is unexamined by construction, the entry had never been checked on the one machine that could check it. That exposed the real problem, which is that UNPROBEABLE was being used for two different things. probe_call already records an ImportError as undecided rather than failed, so an entry that merely needs an optional backend can carry a real case and be skipped where the backend is absent. Excluding it instead means it goes unchecked everywhere, including where it would work. mv.mol.functional_groups, mv.feat.soap and mv.feat.matminer are now probed. Simulating an environment with every optional backend installed leaves no UNPROBEABLE reason that would go stale; what remains there is what no environment can decide - a network call, a real VASP or LOBSTER output, a scheduler, a binary on PATH. The distinction is written at the top of the list so the next entry lands in the right place. 972 tests pass on py3.12.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An entry named in
UNPROBEABLEis unexamined by construction, so the reason has to keep being true.mv.elec.transportwas excluded because BoltzTraP2 was "absent in this environment". Installing IFermi pulled BoltzTraP2 in as a dependency, the reason quietly stopped being true, and nothing noticed — which is how a tutorial calling that same function with the bands-axis object where a list ofBandStructures belongs survived unseen, itsImportErrorswallowed by the cell'sexcept.The new test fails when a reason claims a package is missing and that package imports. Verified by writing a stale reason deliberately, watching it fail, and reverting.
The transport reason now says what is actually true: the package is importable, its top level touches no netCDF, and
BoltzTraP2.dft— which the transport path reaches — raisesnumpy.dtype size changedbecause netcdf4 was built against a different numpy. Still a valid exclusion, for a different reason than the one recorded.This came from a scan, not from stumbling on it again
Two other sweeps found nothing, which is worth recording so nobody repeats them:
requests→urllib,matplotlib_inline→IPython).The stale-reason case was the only real instance. The guards themselves are in good shape; the gap was in the list of things deliberately not checked.
Verification
972 pass on py3.12, 852 on py3.10.
🤖 Generated with Claude Code
https://claude.ai/code/session_016wh5NzNE9Wn49nuUm7zg2a