Turn pylint back on, and triage what it finds

[elinscott]

The pre-commit job has never run its hooks — it invokes pre-commit run with nothing staged, so every hook skips and the job passes. That is fixed separately in #108; this issue is about the one hook that fix leaves switched off.

Running pre-commit run --all-files against main gives 37 pylint findings across 21 modules. Turning the hook on in the same pull request would have meant either fixing all of them blind or leaving CI red, so it is disabled there with a comment, and the work is recorded here.

Two separate questions:

• Which pylint? The hook is configured language: system, so it runs whatever the [pre-commit] extra installs — currently pylint>=3, which resolves to 4.0.7. This codebase has never been through pylint 4, so some part of those 37 findings is a new major version's opinions rather than drift in the code. Pinning the hook to a known version would make the count mean something and stop it moving underfoot.

• Eight of the findings look like real bugs. Most of the 37 are style — too-many-positional-arguments, an unnecessary elif — but these are possibly-used-before-assignment or used-before-assignment, which is pylint reporting a path where a name is read before every branch has bound it:

  • workflows/optimize.py (two findings)
  • workflows/bands.py
  • utils/parser/center.py
  • utils/pseudo/upf.py
  • utils/workflows/plot/distance.py (three findings)

Suggested order:

1. pin the pylint version
2. triage the eight above and fix/dismiss each
3. clear the remaining style findings
4. drop the pylint exclusion from .pre-commit-config.yaml

[elinscott]

AI report as follows for reference:

Triage of the eight used-before-assignment findings, against main at 2d61051 with pylint 4.0.7 (what the unpinned pylint>=3 hook resolves to today), Python 3.11, aiida-wannier90 2.2.0 and aiida-quantumespresso<4.15. All eight reproduce at exactly the lines listed above, so the enumeration is stable. Everything below is graded reproduced (I ran the code and saw the exception) or code-read (I traced the branches but did not drive the path).

#	Location	Name	Verdict
1	workflows/optimize.py:591	last_calc	Unreachable in practice (code-read)
2	workflows/optimize.py:884	last_calc	Unreachable in practice (code-read)
3	workflows/bands.py:415	w90_bands	Real crash, latent — mechanism reproduced, trigger code-read
4	utils/parser/center.py:92	num_pts	Unreachable — guard is exhaustive (reproduced)
5	utils/pseudo/upf.py:341	l	Unreachable — dead code after raise (reproduced)
6	utils/workflows/plot/distance.py:83	mapping	Genuine bug (reproduced)
7	utils/workflows/plot/distance.py:174	fermi_energy	Genuine bug (reproduced)
8	utils/workflows/plot/distance.py:174	exclude_list_dft	Genuine bug (reproduced)

#107 does not touch any of these. It rewrites utils/bands/distance.py; findings 6-8 are in utils/workflows/plot/distance.py, which is byte-identical between main and the #107 branch, and pylint reports the same three there on both. #107's own rewritten module is clean of E06*.

6-8: bands_distance_for_group reads three names that no branch has bound

mapping is bound only when wan_group.nodes[0].process_class != Wannier90OptimizeWorkChain, but it is read for any group member that is not an Optimize workchain carrying optimize_reference_bands. Since that input is optional, a group of Optimize workchains submitted without reference bands satisfies the first condition and fails the second: UnboundLocalError: cannot access local variable 'mapping' where it is not associated with a value. Smallest trigger: a group whose first node is a finished Wannier90OptimizeWorkChain with no optimize_reference_bands input. Negative control: the same fixture with that input present runs to completion, so the crash is pinned to exactly this condition and not to the stub scaffolding.

The fermi_energy / exclude_list_dft pair has the same shape one branch further down: they are bound under process_class == Wannier90Calculation and under process_class in (Wannier90BandsWorkChain, Wannier90OptimizeWorkChain), and there is no else. A plain Wannier90WorkChain — the package's own main entry point — matches neither, so a group of those crashes at the bands_distance(...) call. bands_wannier_node is unbound on the same line for the same reason and pylint does not flag it; it is evaluated first, so that is the name that actually appears in the traceback.

The crash is the benign case. Because these are function locals inside a for loop, a group that mixes supported and unsupported workchain types does not crash at all — the second iteration finds the names still bound from the first and silently pairs the new structure's DFT bands with the previous structure's Wannier bands, Fermi energy and exclude list. I reproduced that: given [Wannier90Calculation(pk 101), Wannier90WorkChain(pk 102)], the second bands_distance call receives dft='<DFT bands 202>' with wannier='<w90 bands 101>' and fermi=6.5 carried over from node 101, and the resulting DataFrame row looks perfectly ordinary. Silencing this by initialising the names to None above the loop converts the silent wrong answer into a TypeError deeper inside bands_distance, which is not much better. The correct behavior is an explicit else that reports the unsupported process class and continues to the next group member, mirroring how the function already handles an unsupported DFT node type a few lines above (raise ValueError(f"Unsupported node type ...")) and how it already skips members whose DFT partner is missing.

3: Wannier90BandsWorkChain.results assumes interpolated_bands exists

w90_bands is bound only inside if "interpolated_bands" in self.outputs[...], in both the collinear and non-collinear branches, and self.out("band_structure", w90_bands) sits outside both. Calling results() with a wannier90 output namespace that lacks interpolated_bands raises UnboundLocalError: cannot access local variable 'w90_bands' where it is not associated with a value (reproduced; with the output present, the same probe emits band_structure normally). The collinear branch is stricter still — it needs both spin channels to have interpolated bands, so a run where only wannier90_down is missing them crashes too.

Whether a supported submission reaches that state is the part I could not settle, so I am not calling this a live bug. prepare_wannier90_pp_inputs forces bands_plot = True and setup always ends with a k-path (seekpath when neither kpoint_path nor bands_kpoints is given), and the main run copies the postproc parameters, so the ordinary path does produce interpolated bands. What makes it fragile rather than safe is that nothing enforces it: interpolated_bands is declared required=False on Wannier90Calculation, and inspect_wannier90 checks only is_finished_ok, so any successful wannier90 run whose retrieved folder has no <seedname>_band.dat walks straight into the unbound read. One concrete route (code-read, not driven): with bands_kpoints supplied instead of kpoint_path, and a wannier90 older than 3.0 that writes no _band.labelinfo.dat, the parser's legacy fallback looks for a kpoint_path input, does not find one, and emits nothing at all while the calculation still finishes ok.

The fix is not w90_bands = None, because band_structure is a required output of this workchain — returning without it fails the process anyway, just with a less legible message. It wants a declared exit code for "the wannier90 run produced no interpolated bands", returned from results() (or better, checked in inspect_wannier90, which is where the other sub-process failures are caught).

1-2: last_calc in optimize.py

Both sites bind last_calc under if self.ctx.current_spin == "up" / elif == "down" with no else, guarded by if self.ctx.spin_collinear. The invariant is one level up rather than in the function: the only callers reached when spin_collinear is true are run_wannier90_optimize_up/down and run_wannier90_plot_up/down, and each is invoked on the line immediately after self.ctx.current_spin is assigned the literal "up" or "down". Every assignment to current_spin in the package is one of those two literals. So it holds, but it is an invariant a reader has to reconstruct, not one the compiler or the type checker keeps. wannier90.py:1127-1137 already writes the same dispatch with else: raise ValueError(f"Unsupported spin component {self.ctx.current_spin}. Expected 'up' or 'down'.") — copying that into both sites states the assumption and silences pylint in the same stroke, which is strictly better than last_calc = None.

4-5: not bugs

In generate_supercell, num_pts is bound under if dimension == 2 / elif dimension == 3, and twenty lines above the function raises NotImplementedError for anything else — reproduced: len(cell[0]) of 1 or 4 raises before line 92, 2 and 3 return supercells of shape (9, 2) and (27, 3). This is a same-function guard, about as robust as such invariants get. a3 at line 79 has the identical shape and pylint does not flag it.

In parse_pswfc_energy_nosoc, line 341 sits after an unconditional raise NotImplementedError with a # pylint: disable=unreachable right beneath it — reproduced: an old-format PP_PSWFC raises NotImplementedError and never reaches it. The block is also harmless on its own terms, because the s/p/d/f chain is exhaustive over what re.compile(r"[\d]([SPDF])") can yield; the live twin of this block, parse_pswfc_nosoc line 284, parses 3S 3P 3D 4F into [{'l': 0}, {'l': 1}, {'l': 2}, {'l': 3}] without complaint — and pylint flags the dead copy while missing the live one. The right move is to delete the dead branch rather than initialise l.