pymatgen icon indicating copy to clipboard operation
pymatgen copied to clipboard

[Dev] Multiple dependencies for CI missing

Open DanielYang59 opened this issue 11 months ago • 5 comments

Multiple external dependencies are missing from test setup, including:

  • [x] mcsqs: not planned (https://github.com/materialsproject/pymatgen/issues/3684#issuecomment-1988998112)
  • [x] icet: not planned (https://github.com/materialsproject/pymatgen/issues/3684#issuecomment-1987687507)
  • [x] emmet: not planned (https://github.com/materialsproject/pymatgen/issues/3684#issuecomment-1987690470)
  • [x] Open Babel: added in #3729
  • [x] enum_lib: Added for Ubuntu only in #3729
  • [x] gulp: not planned (libgfortran3 not supported by Ubuntu20 afterwards)
  • [ ] graphviz
  • [ ] critic2: https://aoterodelaroza.github.io/critic2/installation/
  • [ ] OBAlign
  • [ ] vampire
  • [ ] x_trans
  • [ ] boltztrap2
  • [ ] zeo

Is this intended or should we fix it?

DanielYang59 avatar Mar 10 '24 04:03 DanielYang59

i believe missing icet was deliberate as @esoteric-ephemera added it only to requirements-optional.txt which is not used in CI.

though i could see how that is confusing which is why i've advocated for removing the requirements files in the past. @shyuep prefers to keep. i fwiw, i think the atomate2 approach is better of having a strict optional deps section in pyproject.toml

janosh avatar Mar 11 '24 06:03 janosh

emmet is purposefully not installed as it comes with a host of transitive deps.

as for the others, i think it would be good to install and test their respective pymatgen integrations in CI

janosh avatar Mar 11 '24 06:03 janosh

i believe missing icet was deliberate as @esoteric-ephemera added it only to requirements-optional.txt which is not used in CI.

That was deliberate. The same logic applies to mcsqs and a few other packages here. Tho that tends to mean that tests for these aren't actively maintained since CI never runs them (could suddenly have broken tests / interfaces if breaking changes are made in a dependency)

esoteric-ephemera avatar Mar 11 '24 17:03 esoteric-ephemera

Tho that tends to mean that tests for these aren't actively maintained since CI never runs them (could suddenly have broken tests / interfaces if breaking changes are made in a dependency)

But I assume it's still beneficial to keep the tests alive for robustness? (Issues are more likely to slip in if tests are skipped)

Anyway, I would try to install some of these dependencies locally and see if any broken tests show up, and I would keep you updated.

DanielYang59 avatar Mar 13 '24 09:03 DanielYang59

@DanielYang59 feel free to install some of the missing packages in CI as well to activate those tests. ideally, we don't want to be blocked by upstream packages in our development (see e.g. tblite) but if a package is actively maintained and easy to install, it makes sense to add it in CI

janosh avatar Mar 13 '24 09:03 janosh