gafusion / omas

Ordered Multidimensional Array Structure
http://gafusion.github.io/omas
MIT License
32 stars 15 forks source link

Remove pygacode as a required dependency #232

Closed LiamPattinson closed 1 year ago

LiamPattinson commented 1 year ago

I've been trying to use some OMAS functions in Pyrokinetics, and although everything worked fine on my machine, it caused our automated tests to fail. It looks like the same problem that was encountered in this issue, in which the build-time numpy.distutils requirement of pygacode was causing the install to fail.

As pygacode isn't used directly by OMAS (as far as I can tell), here I've simply removed it as a required dependency. It's still available as part of the 'machine' extras, though I've also updated that to also not be a required install. To fix this issue in pygacode, somebody will simply need to add a pyproject.toml, as described in this comment.

A concern I have is that these changes make omfit_classes an optional install, and although OMAS is written to permit this, other users may not be using pip install omas[machine] in their workflows and therefore this may break their automated systems. Should omfit_classes be moved from extras_require.machine to install_requires? As far as I can tell, neither pexpect nor fortranformat from extras_require.machine are currently in use. The simplest fix would be to move omfit_classes to install_requires and remove the other dependencies.

Is all of this okay? I'm not sure how OMAS is used by the majority of users, so I'm aware that I may be missing some context regarding the inclusion of pygacode as a dependency.

LiamPattinson commented 1 year ago

I had a go at getting the automated tests to work, and while they are running now, I'm not familiar enough with the structure of the library to diagnose the problem here.

orso82 commented 1 year ago

@LiamPattinson thank you for fixing the OMAS regression tests! I have cherry-picked those changes and now testing not having omfit_classes pexpect fortranformat pygacode be required packages (see https://github.com/gafusion/omas/pull/233)