fyalcin / surfflow

High-throughput workflows to calculate surface energies of solids.
MIT License
7 stars 6 forks source link

Some issues regarding the dependency packages of this package. #3

Open hongyi-zhao opened 2 weeks ago

hongyi-zhao commented 2 weeks ago

Hi there,

This package is wonderful, but regarding the so many dependency packages, it has led to some aspects in configuration and usage that I find very tricky:

  1. A large number of dependencies are listed in requirements.txt. Are they all necessary?
  2. Some of the dependency packages used by this package are clearly outdated, for example:

(a) pymatgen==2023.12.18 (b) Using atomate instead of atomate2

I'd like to know the reasons behind these dependency version choices during development. For pymatgen, I can remove the version restriction above manually, but for atomate2, it's not that simple because they are two different packages with significant differences in their specific APIs.

See here for the related discussion.

Regards, Zhao

fyalcin commented 2 weeks ago

Hey Zhao,

Thanks for your continued interest and compliment :)

The exact version pinning in the requirements.txt is only provided as a way to closely reproduce the python environment I used when calculating the surface energies in the paper. I agree that this is probably overkill but it doesn't hurt if someone wants the option, since installing the surfflow from PyPI (or locally editable via pip install -e ., quite useful for development) only installs the dependencies in the pyproject.toml, and in this case the requirements.txt does not even enter the equation.

Recently someone else was interested in the package and they were having some issues when they installed it from PyPI. I located the cause to be a change in the dependencies in the newer recent pymatgen versions, and since I really don't have time to fix surfflow to accommodate these changes, I had to pin the version of pymatgen. This time constraint is also the reason why I still use atomate instead of atomate2.

From the matsci thread, my guess is that you would like to build another package that will work with surfflow, and I fully understand why you don't want to use atomate, when it's being phased out. That being said, technically it shouldn't be too difficult to update surfflow to work with atomate2, as the developers of atomate2 conveniently provide ways to turn the flows (since it uses jobflow instead of fireworks by default) to fireworks. I can not promise a timeframe but I will try to take a look at a possible transition when I find the time.

hongyi-zhao commented 2 weeks ago

and in this case the requirements.txt does not even enter the equation.

Do you mean the equation of conda dependency solver?

Recently someone else was interested in the package and they were having some issues when they installed it from PyPI. I located the cause to be a change in the dependencies in the newer recent pymatgen versions,

Can you provide precise test cases that trigger this problem?

fyalcin commented 2 weeks ago

I meant equation as in the requirements.txt is not even used at any point unless someone explicitly runs pip install -r requirements.txt after cloning the repo.

So far I've always installed surfflow myself from PyPI and apart from the recent pymatgen and yaml loading issues (both of which should now be fixed) it just worked. I don't remember now what the dependency issue with pymatgen exactly was, but after some trial and error I settled in the specific pymatgen version you see in the pyproject.toml.

hongyi-zhao commented 2 weeks ago

but after some trial and error I settled in the specific pymatgen version you see in the pyproject.toml.

The development and iteration of pymatgen is very frequent. Pinning to such an old version as you have set could mean potential impacts from many bugs that have already been fixed and the inability to use many new features.

fyalcin commented 2 weeks ago

I agree, but I am just one person and keeping the dependencies of this package up-to-date unfortunately ranks quite low in my priorities, especially when doing so will not bring any improvements to the workflow. I am fairly certain that bugfixes and new features that come with newer pymatgen versions will not have a meaningful effect on what this package aims to achieve, which is calculating surface energies in an automated fashion. If you need access to a newer pymatgen version while using this package, you might want to implement the neccesary changes yourself, and if you decide to share those changes, I'd welcome any pull request you make.

hongyi-zhao commented 2 weeks ago

I see, let's keep in touch and maintain possible communication.