Fix failing tests and migrate to importlib #216
Conversation
importlib.resources.files is only implemented in Python >= 3.9 Using the backport works as an alternative until sumo drops python 3.8 support
This makes it easier/cleaner to use importlib.resources.files, and seems to fix some (but not all) local issues. It also appears to be recommended in PyPI's sample package.
Tests were passing locally but not on GitHub actions, is pytest not case sensitive on MacOS but case sensitive on Linux? Doesn't make sense...
Move to using importlib.resources (standard library) instead of importlib_resources (backport) if it's available. Also rename the files import to ilr_files to avoid ambiguity.
* Move the rest of the code to importlib.resources * A little bit of linting, focused on line lengths
* Use more recent versions of actions to get rid of deprecation warning * Add python3.11 test * Use setup-python's caching features instead of caching action
|
Thank you for contributing this significant maintenance work! I will try to review when possible; hopefully this will make it easier to progress some of the other open PRs... |
| help=( | ||
| "orbitals to split into lm-decomposed " | ||
| 'contributions (e.g. "Ru.d")' | ||
| ), |
There was a problem hiding this comment.
I don't understand what is going on with the linting here.
Obviously a human would never have written the existing version. Did some other auto-linter remove the line-break? And if so, did it break PEP8 in the process? @utf any idea?
This is why I dislike auto-formatters...
There was a problem hiding this comment.
I checked the blame, and indeed this was ruined by black. The original version was
parser.add_argument('--orbitals', type=_el_orb, metavar='O',
help=('orbitals to split into lm-decomposed '
'contributions (e.g. "Ru.d")'))
|
Tests are passing, which is great! I am finding this difficult to review because there are so many unrelated formatting changes mixed into the diffs. Are these necessary in order to make the tests pass, or can we split them into a separate PR? |
There was a problem hiding this comment.
Ok, I have slogged through the indentation noise and the actual changes seem fine.
The main change request is that importlib.resources.files should return a "Traversable" which can be extended with an elegant / syntax. We should replace most of the instances of path.join with this syntax. There may be places that this causes a problem because something was expecting a string: usually the better solution will be to make it accept a Traversable, but casting with str is also an option.
This line-length formatting business is very tedious and will create merge conflict hell for other open Pull requests. I don't really care what our max line length is (within reason) but it is obviously bad that automatic formatters are choosing their own. @utf how do we stop this from happening? I guess we should add some kind of linting to the CI.
Either we accept the changes here and revert to 79-characters, or we need to run black on this branch with whatever settings were previously used and hopefully it will undo some of this cruft.
Either way, we are left with weird mixtures of "" and '' and dangling ) all over the place, which makes me sad 😞
| config_path = resource_filename( | ||
| Requirement.parse("sumo"), "sumo/plotting/orbital_colours.conf" | ||
| config_path = os.path.join( | ||
| ilr_files("sumo.plotting"), "orbital_colours.conf" |
There was a problem hiding this comment.
I think with importlib.resources.files we can have files("sumo.plotting") / "orbital_colours.conf" which is a bit cleaner.
| ) | ||
| parser.add_argument( | ||
| "-c", | ||
| "--code", | ||
| default="vasp", | ||
| metavar="C", | ||
| help=('Input file format: "vasp" (vasprun.xml) or ' '"questaal" (opt.ext)'), | ||
| help=( | ||
| 'Input file format: "vasp" (vasprun.xml) or ' '"questaal" (opt.ext)' |
There was a problem hiding this comment.
More absurdity from linters. This should be a single string.
| config_path = resource_filename( | ||
| Requirement.parse("sumo"), "sumo/plotting/orbital_colours.conf" | ||
| config_path = os.path.join( | ||
| ilr_files("sumo.plotting"), "orbital_colours.conf" |
There was a problem hiding this comment.
Again, I think the join can become a /.
| ) | ||
| sumo_optics_style = os.path.join( | ||
| ilr_files("sumo.plotting"), "sumo_optics.mplstyle" | ||
| ) |
There was a problem hiding this comment.
Again I think these joins can be cleaner / expressions. One thing to watch for is that we might need to convert the result to a string: there is some minimum version of Matplotlib that will accept resource paths as style files and before that they have to be converted to str.
We could also cut down on boilerplate by assigning a variable so the block becomes
_sumo_plotting = ilr_files("sumo.plotting")
sumo_base_style = str(_sumo_plotting / "sumo_base.mplstyle")
sumo_dos_style = str(_sumo_plotting / "sumo_dos.mplstyle")
...There was a problem hiding this comment.
I wouldn't object to losing the str and bumping the matplotlib requirement to 3.2.0, that's what I did in Euphonic!
| @@ -78,7 +85,7 @@ def _get_bradcrack_data(bravais): | |||
| 'path': [['\Gamma', 'X', ..., 'P'], ['H', 'N', ...]]} | |||
|
|
|||
| """ | |||
| json_file = pkg_resources.resource_filename(__name__, "bradcrack.json") | |||
| json_file = os.path.join(ilr_files("sumo.symmetry"), "bradcrack.json") | |||
Hi again,
This PR moves sumo from using
pkg_resourcestoimportlib.resources, which fixes the failing tests. Note thatpkg_resourcesis now deprecated and the devs recommend migrating toimportlib.resourcesor theimportlib-resourcesbackport for older versions of python.Changelog:
pkg_resourceswithimportlib.resourcesfor python 3.9+ and theimportlib_resourcesbackport for python 3.8. This fixes the issue of failing tests.testsinto its own package for easier use withimportlib.resources.files, and add proper exclusion tosetup.py