Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up RNN branch #13

Closed
APJansen opened this issue Oct 24, 2022 · 7 comments · Fixed by #32
Closed

Clean up RNN branch #13

APJansen opened this issue Oct 24, 2022 · 7 comments · Fixed by #32
Assignees

Comments

@APJansen
Copy link
Contributor

APJansen commented Oct 24, 2022

  • Remove unnecessary files
  • Remove/merge duplicate boilerplate files
  • Change file structure to merge with grainlearning
  • Update requirements
  • etc
@APJansen APJansen self-assigned this Oct 24, 2022
@APJansen
Copy link
Contributor Author

With poetry we probably want to make the rnn dependencies optional, as they are quite heavy (mainly tensorflow).

@APJansen
Copy link
Contributor Author

I manually updated pyproject.toml adding the rnn dependencies. For the versions I checked what the latest was and used that, but that might not be necessary. Then ran poetry lock --no-update to add these to the lock file without upgrading versions of other packages. Then tested installing with poetry install, but there is an issue with tensorflow, specifically: Unable to find installation candidates for tensorflow-io-gcs-filesystem (0.27.0) (related issue)
Likely this is because I'm on a (arm) mac, replacing tensorflow with tensorflow-macos fixed it for me, everything now installs. But this needs to be fixed for everyone somehow..

@APJansen
Copy link
Contributor Author

It runs now, as long as the data is present at grainlearning/rnn/data/sequences.hdf5. Wandb creates a folder inside rnn/.

@APJansen
Copy link
Contributor Author

Another todo: generate the documentation, and probably for that to work we have to pick one convention for docstrings as currently we're using different ones.

@chyalexcheng chyalexcheng self-assigned this Oct 30, 2022
@luisaforozco luisaforozco self-assigned this Dec 7, 2022
@luisaforozco
Copy link
Collaborator

We picked the following convention for docstrings:

  • Use python type hints.
  • Use sphinx format for docstrings, specifically:
    :param : and :return :, we are dropping :type : and :rtype:

@luisaforozco
Copy link
Collaborator

Regarding the dependencies, In this commit I created two groups of extras rnn and rnn_M1_macOS with the necessary dependencies (only difference is in tensorflow).
I tested in my machine poetry install -E rnn_M1_macOS and on Snellius poetry install -E rnn, and the dependencies are handled correctly.

@luisaforozco
Copy link
Collaborator

The installation of tensorflow for arm64 macOS requires some extra steps that cannot be handled from poetry. I think the best is to keep it separately: if an user has M1 macOS he/she will have to install tensorflow first on his own, and then he/she can install grainLearning on the same python environment.

@luisaforozco luisaforozco linked a pull request Mar 10, 2023 that will close this issue
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 a pull request may close this issue.

3 participants