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

Ajout d'outils et modèles simples de LSTM #82

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Conversation

richardarsenault
Copy link
Contributor

Ajout d'outils de LSTM pour xhydro.

  • Ajout de tensorflow dans les dépendances
  • Ajout de codes et données simples pour entrainement. Il faudra bouger ces données vers xhydro-testdata éventuellement
  • Ajout de tous les outils pour le batching, création des tensors et fonctions objectifs en format Keras
  • Ajout de tests simples

Il s'agit d'une première itération qui nécessitera des passes supplémentaires pour la doc, des tests et d'autres exemples d'application. Il faudra certainement qu'un expert aide à améliorer le code. Pour l'instant, les tests passent tous sous Pycharm mais pas avec pytest, peu importe ce que j'essaie. Surement une erreur usager mais ce PR m'aidera à voir s'ils passent dans CI ou pas...

@sebastienlanglois
Copy link
Contributor

@richardarsenault Tu as l'option de créer un Draft PR si tu veux ouvrir une PR sans pour autant que ce soit prêt pour révision.

@richardarsenault
Copy link
Contributor Author

Merci pour l'info! Je ne savais même pas que ça existait! Je vais trouver comment et faire ça la prochaine fois.

A+!

@sebastienlanglois
Copy link
Contributor

Tu peux encore le faire, il n'est pas trop tard! Juste à cliquer sur le bouton ici :
A+

image

@richardarsenault richardarsenault marked this pull request as draft February 6, 2024 00:11
@richardarsenault
Copy link
Contributor Author

... Tu comprends maintenant pourquoi je suis plus ou moins à l'aise avec les classes et les éléments plus avancés :D

Merci!

tests/LSTM_test_data.nc Outdated Show resolved Hide resolved
@Zeitsperre
Copy link
Collaborator

Zeitsperre commented Mar 7, 2024

Ignore the Python3.12 build failure. I'll fix that soon.

Update: Never mind, tensorflow does not support Python3.12 on conda-forge (ref)

@github-actions github-actions bot added the CI label Mar 7, 2024
@richardarsenault
Copy link
Contributor Author

@Zeitsperre Indeed, Tensorflow is a PITA for this.

  1. It only works on Linux (and I think Mac);
  2. It can only be run efficiently on systems with NVIDIA CUDA-capable GPUs or TPUs, with the proper CuDNN libraries and GPU drivers, which need to be stable and compatible with the GPU version;
  3. CPUs can be used but they need to have the AVX and AVX2 instruction sets, which is not a given;
  4. It requires a specific version of numpy for each specific version of Tensorflow, which must all be compatible with CuDNN, Drivers and GPU versions.

It's a real pain, but when it works, it's amazing. I am starting to wonder how nicely this will play within xhydro and if we should consider making it a spinoff package...

@Zeitsperre
Copy link
Collaborator

@richardarsenault

Not sure what you mean by spinoff package, do you mean to make these functions into their own library that is independent of xhydro or into a library that will be listed as a dependency of xhydro?

Thanks to all the configuration work performed for xhydro and xdatasets, we have a nice template if we want to start/spin off new projects, but I'm cautious to build a sprawling network of dependencies unless it makes the most sense.

I do agree that the unique characteristics of working with CUDA suggest that this functionality/module would only be useful for a specific configuration. There are ways of making a module more "interdependent" (i.e. optional/conditional) instead of independent, but it depends on how we see it fitting into the grand scheme.

@RondeauG thoughts?

@richardarsenault
Copy link
Contributor Author

@Zeitsperre Yes, what I meant was for it to be it's own package, one that could perhaps pull-in a specific version of xhydro or something to keep things compatible. I really don't know enough to examine the options and know what's possible, but my fear is that this might become a major pain point later, say if we start upgrading python versions or other packages. If there are ways around that then great, but I just wanted to shine light on this as I think it might be an issue later on.

thanks!

Copy link
Collaborator

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

J'ai l'impression qu'il y a probablement un grand dédoublement de code entre les versions de base et local de plusieurs fonctions, qui pourrait être mieux optimisé. Mais à part ça (et ce n'est vraiment pas prioritaire), ça me semble bon !

LSTM_test_data.nc Outdated Show resolved Hide resolved
LSTM_test_data_local.nc Outdated Show resolved Hide resolved
xhydro/lstm_tools/LSTM_static.py Outdated Show resolved Hide resolved
xhydro/lstm_tools/LSTM_static.py Outdated Show resolved Hide resolved
xhydro/lstm_tools/create_datasets.py Outdated Show resolved Hide resolved
xhydro/lstm_tools/lstm_functions.py Outdated Show resolved Hide resolved
xhydro/lstm_tools/lstm_functions.py Outdated Show resolved Hide resolved
@richardarsenault richardarsenault marked this pull request as ready for review March 23, 2024 17:57
@richardarsenault
Copy link
Contributor Author

Je crois que ce package est prêt à être déployé. Cependant, @Zeitsperre et @RondeauG , peut-être qu'il serait mieux (tel que discuté) de sortir ce package de xhydro en raison des complexités de tensorflow?

On pourrait aussi le laisser tel quel et si l'usager n'a pas de GPU, ça va reverter au CPU automatiquement SI le CPU a un set d'instructions AVX et AVX2. Sinon ça va planter. On pourrait peut-être ajouter un test qui vérifie si ce critère est respecté puis skipper les tests si aucune méthode n'est dispo?

De cette manière, un usager pourrait quand même l'utiliser s'il a le hardware et l'environnement NVIDIA/CUDA requis et on pourrait laisser ça dans le package xhydro. Sinon, si on le sort pour en faire un package standalone c'est correct aussi, mais je vais attendre avant de faire un Notebook explicatif dans ce cas là car ça ne sera pas le même scope.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the notebooks Run tests against notebooks label Mar 24, 2024
Updated to reflect path of merged test data.
@sebastienlanglois
Copy link
Contributor

Nous, on a bien hâte de tester avec nos clusters GPU :)

@richardarsenault
Copy link
Contributor Author

@sebastienlanglois
Moi aussi, et je peux vous aider à monter des structures intéressantes (plus que ce qu'il y a dans le package pour l'instant)!

Copy link
Collaborator

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

Ça me semble bon ! Tu peux considérer ceci comme un Approved 😄

On pourra voir pour créer un repo spécifique aux LSTMs.

@richardarsenault
Copy link
Contributor Author

OK merci!

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

Successfully merging this pull request may close these issues.

4 participants