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

Sc1 demo #20

Merged
merged 17 commits into from
Mar 8, 2024
Merged

Sc1 demo #20

merged 17 commits into from
Mar 8, 2024

Conversation

ahandan
Copy link
Contributor

@ahandan ahandan commented Nov 22, 2023

Version bump to 0.3.1

  • Updating the environnement to add the requirements for the next two notebooks to work.

Adding 2 notebooks :

  1. Sc1-Demontrator notebook of a complete workflow as example for spatial imagerie analysis and process execution.
  2. Process Management notebook to help user understand the usage of weaver and how to manage deployed process.

Relates to DAC-466

@ahandan ahandan requested a review from ChaamC November 22, 2023 15:47
@ahandan
Copy link
Contributor Author

ahandan commented Nov 22, 2023

@ChaamC Pour le changelog et le version bump, je suis pas certain de suivre le bon standard.

Si on bump de version à 0.3.1 c'est également le latest, on est d'accord ou je fais une erreur ?

@ChaamC
Copy link
Collaborator

ChaamC commented Nov 23, 2023

@ChaamC Pour le changelog et le version bump, je suis pas certain de suivre le bon standard.

Si on bump de version à 0.3.1 c'est également le latest, on est d'accord ou je fais une erreur ?

Je dirais que le latest est vraiment la dernière version actuelle du code. Si on bump la version et que c'est le dernier changement, ça sera aussi le latest. Si on met des changements intermédiaires entre des versions (même si pas usuel de faire ça), le latest pourrait devenir différent de la version.

Copy link
Collaborator

@ChaamC ChaamC left a comment

Choose a reason for hiding this comment

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

Je n'ai pas révisé le contenu des notebooks, je pense qu'ils ont déjà été révisé ailleurs ou je me trompe? Sinon, laisse-moi savoir si tu veux que je les review.

Est-ce que ce serait possible aussi de bumper l'image NLP en même temps? Ce serait pour les changements ajoutés par Nazim. Je pense qu'on peut la mettre à 0.4.1 celle-là (bump patch only), vu qu'il n'y a pas de nouveau contenu, mais juste un fix et un bump du base image.

eo/CHANGELOG.md Outdated
@@ -3,11 +3,14 @@ Changes

Unreleased (latest)
===================

0.3.1 (2023-11-22)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je mettrais peut-être 0.4.0 au lieu de 0.3.1 ici, vu qu'on ajoute du nouveau contenu , incluant de nouveaux notebooks et de nouveaux requirements.

Copy link
Contributor Author

@ahandan ahandan Nov 23, 2023

Choose a reason for hiding this comment

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

Oui je suis d'accord avec toi.
Effectivement, puisque NLP est à 0.4.0, je suis de ton avis et ça va permettre de synchronizer les deux image.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Juste pour être sûr pour les bumps, en résumé :

  • eo : 0.3.0 -> 0.4.0
  • nlp : 0.4.0 -> 0.4.1

Copy link
Contributor

Choose a reason for hiding this comment

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

nouveau NLP déjà à 0.5.0

pour EO, oui pour 0.4.0
il y a plusieurs nouveaux features/notebooks, pas simplement des bugfixes

@ahandan
Copy link
Contributor Author

ahandan commented Nov 23, 2023

@ChaamC Pour le changelog et le version bump, je suis pas certain de suivre le bon standard.
Si on bump de version à 0.3.1 c'est également le latest, on est d'accord ou je fais une erreur ?

Je dirais que le latest est vraiment la dernière version actuelle du code. Si on bump la version et que c'est le dernier changement, ça sera aussi le latest. Si on met des changements intermédiaires entre des versions (même si pas usuel de faire ça), le latest pourrait devenir différent de la version.

Pas besoin de revoir les notebooks, il sont déjà en revu dans un autre dépôt.
Ici on se concentre sur l'environnement, dockerfile et changelog.

eo/CHANGELOG.md Outdated Show resolved Hide resolved
eo/CHANGELOG.md Show resolved Hide resolved
eo/CHANGELOG.md Outdated Show resolved Hide resolved
eo/notebooks/demo/eo-tools-application.cwl Outdated Show resolved Hide resolved
eo/notebooks/demo/eo-tools-application.cwl Outdated Show resolved Hide resolved
eo/notebooks/demo/eo-tools-application.cwl Outdated Show resolved Hide resolved
nlp/CHANGELOG.md Outdated Show resolved Hide resolved
eo/notebooks/process_management.ipynb Outdated Show resolved Hide resolved
eo/notebooks/process_management.ipynb Outdated Show resolved Hide resolved
Copy link
Contributor

@fmigneault fmigneault left a comment

Choose a reason for hiding this comment

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

Just some minor typos to patch and other comments from previous PR review that were not addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Adjust header to have "DACCS" instead of "Daccs".
  • There is a mention of "2.Deploy the process on weaver from the ./Process_Management_Demonstration notebook". It looks like the notebook is called process_management.ipynb in this repo.
  • Typo "none usage from the users." -> "non-usage from the users"
  • Typo "because of that these sectionsthey can be used as examples." extra "they" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Process_Management_Demonstration, yes, I just renamed the notebook to Process_Management_Demonstration. for more clarity.

Not sure to understand what are the previous PR review that were not addressed?
Are these suggestions the ones you are talking about ?

@fmigneault fmigneault merged commit a583551 into master Mar 8, 2024
2 checks passed
@fmigneault fmigneault deleted the sc1-demo branch March 8, 2024 18:10
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.

4 participants