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

feat(openadapt): add progress bar in record.py and visualize.py #318

Merged
merged 29 commits into from
Jul 3, 2023

Conversation

KrishPatel13
Copy link
Collaborator

@KrishPatel13 KrishPatel13 commented Jun 26, 2023

What kind of change does this PR introduce?
Feature: Add Progress Bar for Record and Visualization

Summary

Issue: #316
Checklist

  • My code follows the style guidelines of OpenAdapt
  • I have perfomed a self-review of my code
  • If applicable, I have added tests to prove my fix is functional/effective
  • I have linted my code locally prior to submission
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
  • New and existing unit tests pass locally with my changes

How can your code be run and tested?

Other information

@KrishPatel13 KrishPatel13 requested review from abrichr and 0dm June 26, 2023 16:18
@KrishPatel13 KrishPatel13 self-assigned this Jun 26, 2023
@KrishPatel13
Copy link
Collaborator Author

Quick Preview of Progress bar in Visualization: #316 (comment)

@KrishPatel13
Copy link
Collaborator Author

KrishPatel13 commented Jun 26, 2023

Quick Preview of Progress Bar in Record: #316 (comment)

@KrishPatel13
Copy link
Collaborator Author

Progress bar with Visualization looks good, but for record I have added the Progress bar in the function write_event

openadapt/record.py Outdated Show resolved Hide resolved
Krish Patel added 2 commits June 26, 2023 12:24
@KrishPatel13
Copy link
Collaborator Author

KrishPatel13 commented Jun 26, 2023

Quick Preview of Progress Bar in Record: #316 (comment)

The messy visualization are due to threads. as the function write_event is called in threads and multiple times as and when the action enveet are added to the write_performace queue. Hence, you will notice the progress bar in multiple times during the logging of record.py.

@0dm
Copy link
Collaborator

0dm commented Jun 26, 2023

we might need to use an event queue to ensure that there's only one progress bar. Or another approach could be to capture stdout while scrubbing (there isn't any useful info anyway it's usually just warnings about the process getting forked)

@KrishPatel13
Copy link
Collaborator Author

we might need to use an event queue to ensure that there's only one progress bar

@0dm This sounds good, But on thinking: For only 1 Progress bar we need a single Loop that iterate over N times. Where N should be predetermined. If we change N then it will be difficult to think of a progress bar. Here in record.py, when writing to db, we do not have any loop which it iterated over. Hence, we might need to design something similar first.

Could you expand on how can we implement this :?

@0dm
Copy link
Collaborator

0dm commented Jun 26, 2023

as said in meeting - we should only use the progress bar after sending termination signal - it'll be cleaner

@KrishPatel13
Copy link
Collaborator Author

Agreed! Working on it

@KrishPatel13
Copy link
Collaborator Author

I am finding it difficult to replicate our requirement with alive-progress bar. The reason is that when want the progress bar disabled before the termination. As soon as record is terminated we want it to be activated and then start. We can disable the progress bar in alive-progress. But currently it does not have the feature to get reactivated/enabled.

Hence, I am now tryin tqdm. I had to spent a lot of time today searching/fixing our use case. I was not able to work on extension due to this :(

@KrishPatel13
Copy link
Collaborator Author

To be consistent with all of the files:

I am using TQDM Progress bar with all Record.py, Visualization.py and Scripts/scrub.py/scrub_mp4

@KrishPatel13
Copy link
Collaborator Author

KrishPatel13 commented Jun 27, 2023

Final Preview for Recrod.py:
Untitled video - Made with Clipchamp

Final Preview for Visualiztion.py:
Untitled video - Made with Clipchamp (1)

--add dynamic_cols: to enable adjustments when window is resized

Order:
--total
-description
--unit
--Optional[bar_format]
--colour
--dynamic_ncols
@KrishPatel13
Copy link
Collaborator Author

@abrichr / @0dm Ready for review!

openadapt/record.py Show resolved Hide resolved
openadapt/visualize.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@KrishPatel13 KrishPatel13 requested a review from 0dm June 28, 2023 15:23
@KrishPatel13 KrishPatel13 linked an issue Jun 28, 2023 that may be closed by this pull request
@0dm
Copy link
Collaborator

0dm commented Jun 28, 2023

visualization is working as expected now, I will try looking into the recording issue since it's still persisting.

@0dm
Copy link
Collaborator

0dm commented Jun 28, 2023

image
looks like qsize is not supported everywhere.

@KrishPatel13
Copy link
Collaborator Author

Thank you @0dm.

Any alternatives you found for Mac in that case ? That serves the same purpose.

@0dm
Copy link
Collaborator

0dm commented Jun 28, 2023

@KrishPatel13 maybe this will be of use?

https://gist.github.com/FanchenBao/d8577599c46eab1238a81857bb7277c9

@KrishPatel13
Copy link
Collaborator Author

@0dm
I have added the fix for MacOs as you gave the link. Please review again 🙏

@0dm
Copy link
Collaborator

0dm commented Jun 29, 2023

works!

openadapt/record.py Outdated Show resolved Hide resolved
openadapt/thirdparty_customization/my_queue.py Outdated Show resolved Hide resolved
openadapt/thirdparty_customization/my_queue.py Outdated Show resolved Hide resolved
@0dm
Copy link
Collaborator

0dm commented Jun 29, 2023

just a few minor changes and I think we'll be good for merging

Copy link
Collaborator Author

@KrishPatel13 KrishPatel13 left a comment

Choose a reason for hiding this comment

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

Self-reviewd!

@KrishPatel13
Copy link
Collaborator Author

@abrichr / @0dm , Ready for Merging!

@KrishPatel13
Copy link
Collaborator Author

@0dm Thank your time and help ;)

@KrishPatel13 KrishPatel13 changed the title Feature/record pb feat(openadapt): Add Progress Bar in record.py and visualize.py Jun 29, 2023
@KrishPatel13 KrishPatel13 changed the title feat(openadapt): Add Progress Bar in record.py and visualize.py feat(openadapt): add progress bar in record.py and visualize.py Jun 29, 2023
@abrichr abrichr merged commit 3e12fd4 into OpenAdaptAI:main Jul 3, 2023
@abrichr abrichr deleted the feature/record_pb branch July 3, 2023 22:49
R-ohit-B-isht pushed a commit to R-ohit-B-isht/OpenAdapt that referenced this pull request Jun 21, 2024
* run `poetry lock --no-update`

* add alive-progress via poetry and in code

* add progress bar in visualization

* add a check for MAX_EVENT = None

* update the title for the Progress bAr
 (better for USer pov)

* update the requirement.txt

* ran ` black --line-length 80 <file>`
on record.py and visualize.py

* remove all progress bar from record

* add tqdm progress bar in recrod.py

* add tqdm for visualiztion

* remove alive-progress

* consistent tqdm api

--add dynamic_cols: to enable adjustments when window is resized

Order:
--total
-description
--unit
--Optional[bar_format]
--colour
--dynamic_ncols

* Update requirements.txt

Co-authored-by: Aaron <57018940+0dm@users.noreply.github.com>

* Address comemnt:
OpenAdaptAI#318 (comment)

* remove incorrect indent

* remove rows

* try to fix distorted table in html

* add custom queue class

* lint --line-length 80

* fix `NotImplementedError` for MacOs
-- using custom MyQueue class

* rename custom -> thirdparty_customization

* rename to something useful

* address comments

* rename dir to customized_imports

* rename to extensions
OpenAdaptAI#318 (comment)

---------

Co-authored-by: Aaron <57018940+0dm@users.noreply.github.com>
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.

Progress bar for recording and visualization
3 participants