search

simonpintarelli / upf_to_json

UPF to json converters
BSD 2-Clause "Simplified" License
5 stars 4 forks source link

Package fails to import at 0.9.4 #1

Closed ml-evs closed 2 years ago

ml-evs commented 2 years ago

Hi there, we've just been hit with an issue due to aiida-core's dependence on this package at optimade-python-tools (CI failure link)

It seems like import upf_to_json fails for v0.9.4:

>>> import upf_to_json
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/mevans/src/upf_to_json/upf_to_json/__init__.py", line 5, in <module>
    from .upf_to_json import upf_to_json
  File "/home/mevans/src/upf_to_json/upf_to_json/upf_to_json.py", line 5, in <module>
    from upf1_to_json import parse_upf1_from_string
ModuleNotFoundError: No module named 'upf1_to_json'

This currently makes aiida-core unusable on fresh installs, perhaps some automated tests would catch this before release? (Happy to help set them up!)

simonpintarelli commented 2 years ago

@ml-evs I'm very sorry for this. I fixed it now. I should have been more careful, especially since this package lacks any sort of tests. The scripts are part of the sirius repository. Yesterday I copied the updated files from sirius and forgot that we had added an extra function for aiida.

What is best to use for the tests? pytest?

ml-evs commented 2 years ago

Nothing to apologize for (and sorry for summoning you on a Sunday)!

I was thinking more of automated tests/scripts that run when you push to this repo, using something like GitHub actions. The tests could be pytest or anything really, do you want me to make a quick pull request as a demonstration?

ml-evs commented 2 years ago

And thanks for the quick fix!

bosonie commented 2 years ago

@simonpintarelli thanks for the quick fix! Just for future references, if a change of API is involved, semantic versioning guidelines suggest to change the major version number (so for initial development phases 0.X.Y, the X should change). This should be enough to avoid problems in depended packages (assuming of course they specify their dependencies following the semantic versioning guidelines).

simonpintarelli commented 2 years ago

@ml-evs, thank you! No problem, also Saturday night would be fine. Breaking aiida-core isn't a petty crime. I've added a github action to check that at least pip install / import works: https://github.com/simonpintarelli/upf_to_json/blob/master/.github/workflows/test.yaml

I'll add pytest later, since we don't test the upf conversion in main repository.

ml-evs commented 2 years ago

Wonderful, thanks. I'll close this then! 👍