Skip to content

Conversation

mmhamdy
Copy link
Contributor

@mmhamdy mmhamdy commented Feb 4, 2024

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

Copy link
Member

@fabianp fabianp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks amazing! Way to go @mmhamdy! 🚀

Just one thing, could you please remove the %pip install statements, and instead add these dependencies to the pyproject.toml file, under section [docs].

@mmhamdy
Copy link
Contributor Author

mmhamdy commented Feb 4, 2024

Sure, much more cleaner this way.

@mmhamdy
Copy link
Contributor Author

mmhamdy commented Feb 4, 2024

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

@fabianp
Copy link
Member

fabianp commented Feb 4, 2024

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

@mmhamdy
Copy link
Contributor Author

mmhamdy commented Feb 4, 2024

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

@fabianp
Copy link
Member

fabianp commented Feb 5, 2024 via email

@fabianp
Copy link
Member

fabianp commented Feb 5, 2024

@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 😉

@mmhamdy
Copy link
Contributor Author

mmhamdy commented Feb 5, 2024

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

@fabianp
Copy link
Member

fabianp commented Feb 7, 2024

BTW I submitted a PR fixing python 3.9 errors: google/grain#338

@mmhamdy
Copy link
Contributor Author

mmhamdy commented Feb 7, 2024

That's QUITE a PR! 😅😅

@fabianp
Copy link
Member

fabianp commented Feb 7, 2024

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)

@mmhamdy
Copy link
Contributor Author

mmhamdy commented Feb 7, 2024

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.

@fabianp
Copy link
Member

fabianp commented Feb 21, 2024

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

@vroulet
Copy link
Collaborator

vroulet commented Apr 18, 2025

For the record, in #1270 we removed this notebook (and others) from the docs building configuration so we don't depend on tensorflow anymore during the continuous integration.
This PR is still great to actually migrate the examples to grain for the sake of users learning how to use grain etc... And by the way, grain now supports mac os (not windows I believe but that's fine). So we could try merging this @mmhamdy if you want

@mmhamdy
Copy link
Contributor Author

mmhamdy commented Apr 23, 2025

Hi @vroulet, yeah sure that would be great! It has been some time! Let's close this PR and I'll open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants