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

Progress bar for storing process #398

Merged
merged 8 commits into from
Aug 11, 2023

Conversation

varrialeciro
Copy link
Contributor

Description

Added a monitoring for percentage monitoring of storing process.
Using the notification list, every 10% of file ingestion, a notification is sent to the admin. The step resolution is variable with parameters "percResolution" that in my PR is set to 10. These are modification core side, the idea (with @Alwe help) could be to modify the admin adding a progress bar that change using the percentage received in the notification.

I opened a new PR because i don't know how can add these modification to the old PR. Sorry :/

Related to issue #(issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

@pieroit pieroit changed the base branch from main to develop August 4, 2023 14:09
@pieroit
Copy link
Member

pieroit commented Aug 4, 2023

Thanks @varrialeciro this is wonderful - I'll take a look soon.

A couple of things:

  • please make your PRs from and to the develop branch
  • do not merge another branch into your feature branch (commits get duplicated)
    💯

@varrialeciro
Copy link
Contributor Author

Yea, i didn't noticed that, thanks for clarifications.

Copy link
Member

@nicola-corbellini nicola-corbellini left a comment

Choose a reason for hiding this comment

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

Here is just a comment for @pieroit about future steps.
Is this is a good moment to think about moving notifications to a dedicated class or maybe it is an overkill?

core/Dockerfile Show resolved Hide resolved
@@ -125,17 +126,25 @@ def ingest_file(
before_rabbithole_stores_documents
"""

print("Start ingestion")
Copy link
Member

Choose a reason for hiding this comment

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

We have an utility to print stuff in the terminal. It would be much appreciated if you please could change this and further print as follows:

from cat.log import log

log(text, "INFO")

there are many logging level available, but I think "INFO" is suitable for the purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merging you can delete that print, were for debugging, I forgot to remove it.

Copy link
Member

Choose a reason for hiding this comment

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

@varrialeciro yes take away the prints please, use log(message, "INFO") :)
Also variables like perc100 and percResolution shoud be snake_case (perc_100, perc_resolution)

Sorry for being picky, we have a lot going on in develop and would like to merge this PR stright away ;)

Thank you for your patience

core/cat/routes/websocket.py Show resolved Hide resolved
@pieroit
Copy link
Member

pieroit commented Aug 9, 2023

Here is just a comment for @pieroit about future steps. Is this is a good moment to think about moving notifications to a dedicated class or maybe it is an overkill?

Thanks for reviewing @nicola-corbellini !
I think it is a good idea to have classes both for notifications and messages, let's merge this and then design them

@@ -125,17 +126,25 @@ def ingest_file(
before_rabbithole_stores_documents
"""

print("Start ingestion")
Copy link
Member

Choose a reason for hiding this comment

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

@varrialeciro yes take away the prints please, use log(message, "INFO") :)
Also variables like perc100 and percResolution shoud be snake_case (perc_100, perc_resolution)

Sorry for being picky, we have a lot going on in develop and would like to merge this PR stright away ;)

Thank you for your patience

@varrialeciro
Copy link
Contributor Author

Updated as you said, removed the debugging print and renamed in snake_case the variables.

@pieroit pieroit merged commit 0417ed9 into cheshire-cat-ai:develop Aug 11, 2023
2 checks passed
@pieroit
Copy link
Member

pieroit commented Aug 11, 2023

Hi @varrialeciro thank you for this, lots of people wanted a progress bar and this is a good move towards it.
The percentages in the notifications went way over 100% (sometime 140%), I tried to fix but I ended up sending notifications based on time: a rabbit thought every x seconds.

Welcome as a contributor :D

Thanks also @nicola-corbellini for reviewing

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