optax icon indicating copy to clipboard operation
optax copied to clipboard

Remove TF dependency in reduce_on_plateau example

Open mmhamdy opened this issue 1 year ago • 12 comments

This PR removes Tensorflow as a dependency in reduce_on_plateau.ipynb, and uses only TFDS, with grain for loading and preparing the dataset.

Addressing issue #656

mmhamdy avatar Feb 04 '24 06:02 mmhamdy

Sure, much more cleaner this way.

mmhamdy avatar Feb 04 '24 13:02 mmhamdy

Hey @fabianp, looks like there is a problem with python 3.9 and grain.

mmhamdy avatar Feb 04 '24 14:02 mmhamdy

ouch! yeah, I'm seeing from https://github.com/google/grain/blob/main/pyproject.toml that they require Python 3.10 ....

fabianp avatar Feb 04 '24 17:02 fabianp

ouch indeed! 😅 What do you think? should we stick to notebook downloads until we get rid of either TensorFlow or Python 3.9?

mmhamdy avatar Feb 04 '24 19:02 mmhamdy

I'm unsure. Let me talk today with the other optax devs to see what they think

On Sun, Feb 4, 2024, 19:50 Mohammed Hamdy @.***> wrote:

ouch indeed! 😅 What do you think? should we stick to notebook downloads until we get rid of either TensorFlow or Python 3.9?

— Reply to this email directly, view it on GitHub https://github.com/google-deepmind/optax/pull/768#issuecomment-1925895311, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDZBYXEP4G7ZGONXUNHQDYR7Q6XAVCNFSM6AAAAABCYTNHXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRVHA4TKMZRGE . You are receiving this because you were mentioned.Message ID: @.***>

fabianp avatar Feb 05 '24 07:02 fabianp

@mmhamdy : I talked today with the pygrain devs, we're going to try to make it work on Python 3.9 . Give me a couple of days to try to get this working 😉

fabianp avatar Feb 05 '24 19:02 fabianp

Sounds great! Also, the MNIST issue has been solved, so I'll be working on the other examples in the meantime.

mmhamdy avatar Feb 05 '24 21:02 mmhamdy

BTW I submitted a PR fixing python 3.9 errors: https://github.com/google/grain/pull/338

fabianp avatar Feb 07 '24 11:02 fabianp

That's QUITE a PR! 😅😅

mmhamdy avatar Feb 07 '24 12:02 mmhamdy

yeah, one thing that worries me a bit more about pygrain is that it it doesn't seem to build on OSX (tried and failed), and probably also Windows (haven't tried there). Before committing replacing TF with pygrain it would be important to make sure that those platforms are supported (just to say that it might take a bit before we merge these PRs unfortunately)

fabianp avatar Feb 07 '24 13:02 fabianp

Well, replacing TensorFlow wasn't expected to be a walk in the park anyway 😃. But we've come a long way and I see that the people at pygrain are working daily on it.

mmhamdy avatar Feb 07 '24 15:02 mmhamdy

thanks for updating @mmhamdy ! Very glad to see that the test now pass!

fabianp avatar Feb 21 '24 08:02 fabianp