search

NNPDF / nnpdf

An open-source machine learning framework for global analyses of parton distributions.
https://docs.nnpdf.science/
GNU General Public License v3.0
30 stars 6 forks source link

Move from `tensorflow.keras` to `keras` #2134

Open scarlehoff opened 4 months ago

scarlehoff commented 4 months ago

This will make the code more general. If we can remove the tensorflow dependency as much as possible (it would be impossible right now to remove it completely) that would be great (it means we could test pytorch for instance).

At the moment (tf 2.17) tensorflow.keras is still provided, but it is not a given that it will continue like that and in particular the conda forge package for macos (maybe a bug on their side, I'm not sure) seems to be missing tensorflow.keras.

RoyStegeman commented 4 months ago

There is no guarantee either way, some years ago this was their position: https://github.com/keras-team/keras/releases/tag/2.3.0

I'd argue that we should keep using tensorflow.keras for possible benefits from better integration with tensorflow. If someone is serious about providing a pytorch backend in n3fit, we can reconsider and in that case swapping out tensorflow.keras for keras should be a quick job.

scarlehoff commented 4 months ago

There is no guarantee either way, some years ago this was their position

I think a change of position in 5 years is still more stable than other programs we use

I've needed to add this already. https://github.com/NNPDF/nnpdf/pull/2110/commits/042d9950889122b729e6e54552a1aa2087ff7c1c and, more generally, I think it is preferable to stick with keras which is actively being developed, that makes sure that the features we use are not deprecated (as opposed to using some tf.keras that hasn't been removed yet when it should).

RoyStegeman commented 4 months ago

Conda packages can always be a mess, separating keras and tensorflow may also result in problems with compatibility between versions.

Feel free to do this if you want to change it, but I don't think that at this point we can say that separating them is clearly the better choice.

scarlehoff commented 4 months ago

separating keras and tensorflow may also result in problems with compatibility between versions.

No. This doesn't generate (new) problems because most of the calls are already using the separate package. If you do from tensorflow import keras you are already getting the separated keras package.

But we risk using features that don't actually exist in the separated keras package (or that have changed name) https://github.com/tensorflow/tensorflow/tree/master/tensorflow/python/keras#stop e.g. https://github.com/NNPDF/nnpdf/blob/4d7284455f4080de0c953350ca742f1830e2b511/n3fit/src/n3fit/backends/keras_backend/MetaModel.py#L24