Revamp the progress reporting API #6556

SpecLad · 2023-07-24T18:03:41Z

Motivation and context

There are several problems with how progress reporting is handled in the SDK, both on the interface and implementation level:

The user is supposed to know, for a given function, which units it will report progress in. This is unnecessary coupling and prevents us from switching to different units, or to have several progress bars using different units.
To create a TqdmProgressReporter, you have to create a tqdm instance, which immediately draws a progress bar. This works poorly if the function prints any log messages before the progress actually starts.
There's no easy way to automatically call finish on a progress bar, so some functions don't (for example, Downloader.download_file). This can cause unexpected output, since tqdm will refresh the progress bar in a background thread (possibly after we've already printed something else).
iter is basically broken, because it divides by period, which is 0 in all current implementations.
Even ignoring that, it's hard to use correctly, because you need to manually call finish in case of an exception.
split is not implemented and not used.
StreamWithProgress.seek assumes that the progress bar is at 0 at the start, and so does ProgressReporter.iter. The former also works incorrectly if the second argument is not SEEK_SET.

Fix these problems by doing the following:

Add a new start2 method which accepts arbitrary tqdm parameters. The default implementation calls start, so that if a user has implemented the original interface, it'll keep working.
Add a TqdmProgressReporter2 that accepts tqdm parameters instead of a tqdm instance, and only creates an instance after start2 is called. Use it where TqdmProgressReporter was used before. The old TqdmProgressReporter is kept for compatibility, but it doesn't support any start2 arguments other than those supported by the original start.
Add a task context manager, which automatically calls start2 and finish. Use it everywhere instead of explicit start/finish calls. Remove start/finish calls from StreamWithProgress and iter.
Implement basic assertions to ensure that start2 and finish are used correctly.
Remove period and split.
Rewrite StreamWithProgress.seek and ProgressReporter.iter to use relative progress reports.

These changes should be backwards compatible for users who pass predefined or custom progress reporters into SDK functions. They are not backwards compatible for users who try to use progress reporters directly (e.g. calling start/finish). I don't consider that a significant issue, since the purpose of the ProgressReporter interface is for the user to get progress information from the SDK, not for them to use it in their own code.

Originally developed for #6483.

How has this been tested?

Checklist

I submit my changes into the develop branch
I have added a description of my changes into the CHANGELOG file
~~[ ] I have updated the documentation accordingly~~
I have added tests to cover my changes
I have linked related issues (see GitHub docs)
[ ] I have increased versions of npm packages if it is necessary
(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)

License

I submit my code changes under the same MIT License that covers the project.
Feel free to contact the maintainers if that's a concern.

codecov · 2023-07-24T18:58:52Z

Codecov Report

Merging #6556 (dfe7a94) into develop (f4a516a) will increase coverage by 0.02%.
The diff coverage is 98.93%.

@@             Coverage Diff             @@
##           develop    #6556      +/-   ##
===========================================
+ Coverage    81.07%   81.10%   +0.02%     
===========================================
  Files          363      363              
  Lines        39632    39644      +12     
  Branches      3563     3563              
===========================================
+ Hits         32132    32153      +21     
+ Misses        7500     7491       -9

Components	Coverage Δ
cvat-ui	`74.77% <ø> (-0.03%)`	⬇️
cvat-server	`86.72% <98.93%> (+0.07%)`	⬆️

zhiltsov-max · 2023-07-26T09:04:14Z

cvat-sdk/cvat_sdk/core/uploading.py

+    @staticmethod
+    def _uploading_task(pbar: ProgressReporter, total_size: int) -> ContextManager[None]:
+        return pbar.task(
+            total=total_size, desc="Uploading data", unit_scale=True, unit="B", unit_divisor=1024


I see several problems with this change:

The implementation supposes that the progress reporter is tqdm

The tqdm has quite a broad list of parameters, it will be hard to implement a custom reporter correctly

The suffix 2 is quite cryptic and tells nothing about the differences, probably it can be replaced with Lazy or Deferred

The implementation supposes that the progress reporter is tqdm

The tqdm has quite a broad list of parameters, it will be hard to implement a custom reporter correctly

That's not quite true, because a reporter doesn't have to support every parameter that tqdm supports. It can simply use whatever parameters it recognizes, and ignore the rest. A barebones reporter doesn't have to use any information other than total (maybe not even total...).

That said, I think I can improve this by explicitly listing the parameters that we actually use right now, and adding a note saying that any kwargs should be ignored. That way, users have more guidance on which parameters they should pay attention to, and there's a way for us to introduce non-tqdm parameters in the future if we need them.

The suffix 2 is quite cryptic and tells nothing about the differences, probably it can be replaced with Lazy or Deferred

The only thing that the 2 is supposed to communicate is that it's better than 1. Which, to me, seems like the main thing that users need to know in this case. If you really want to, I can use Deferred instead.

That said, I think I can improve this by explicitly listing the parameters that we actually use right now, and adding a note saying that any kwargs should be ignored. That way, users have more guidance on which parameters they should pay attention to, and there's a way for us to introduce non-tqdm parameters in the future if we need them.

Sounds good. I think it's the right direction.

If you really want to, I can use Deferred instead.

I won't claim that deferred is the best variant, but 2 feels like "TODO" for me - like if we couldn't find a good name and just left it for future.

Sounds good. I think it's the right direction.

Okay, I did it.

I won't claim that deferred is the best variant, but 2 feels like "TODO" for me - like if we couldn't find a good name and just left it for future.

Mind you, the good name is TqdmProgressReporter - I just couldn't use it because it was already taken. 🙂

Well, I renamed it to DeferredTqdmProgressReporter anyway.

There are several problems with how progress reporting is handled in the SDK, both on the interface and implementation level: * The user is supposed to know, for a given function, which units it will report progress in. This is unnecessary coupling and prevents us from switching to different units, or to have several progress bars using different units. * To create a TqdmProgressReporter, you have to create a tqdm instance, which immediately draws a progress bar. This works poorly if the function prints any log messages before the progress actually starts. * There's no easy way to automatically call `finish` on a progress bar, so some functions don't (for example, `Downloader.download_file`). This can cause unexpected output, since tqdm will refresh the progress bar in a background thread (possibly after we've already printed something else). * `iter` is basically broken, because it divides by `period`, which is 0 in all current implementations. * Even ignoring that, it's hard to use correctly, because you need to manually call `finish` in case of an exception. * `split` is not implemented and not used. * `StreamWithProgress.seek` assumes that the progress bar is at 0 at the start, and so does `ProgressReporter.iter`. The former also works incorrectly if the second argument is not `SEEK_SET`. Fix these problems by doing the following: * Add a new `start2` method which accepts arbitrary tqdm parameters. The default implementation calls `start`, so that if a user has implemented the original interface, it'll keep working. * Add a `TqdmProgressReporter2` that accepts tqdm parameters instead of a tqdm instance, and only creates an instance after `start2` is called. Use it where `TqdmProgressReporter` was used before. The old `TqdmProgressReporter` is kept for compatibility, but it doesn't support any `start2` arguments other than those supported by the original `start`. * Add a `task` context manager, which automatically calls `start2` and `finish`. Use it everywhere instead of explicit `start`/`finish` calls. Remove `start`/`finish` calls from `StreamWithProgress` and `iter`. * Implement basic assertions to ensure that `start2` and `finish` are used correctly. * Remove `period` and `split`. * Rewrite `StreamWithProgress.seek` and `ProgressReporter.iter` to use relative progress reports. These changes should be backwards compatible for users who pass predefined or custom progress reporters into SDK functions. They are not backwards compatible for users who try to use progress reporters directly (e.g. calling `start`/`finish`). I don't consider that a significant issue, since the purpose of the `ProgressReporter` interface is for the user to get progress information from the SDK, not for them to use it in their own code.

## \[2.6.0\] - 2023-08-11 ### Added - \[SDK\] Introduced the `DeferredTqdmProgressReporter` class, which avoids the glitchy output seen with the `TqdmProgressReporter` under certain circumstances (<#6556>) - \[SDK, CLI\] Added the `cvat_sdk.auto_annotation` module, providing functionality to automatically annotate tasks by executing a user-provided function on the local machine. A corresponding CLI command (`auto-annotate`) is also available. Some predefined functions using torchvision are also available. (<#6483>, <#6649>) - Included an indication for cached frames in the interface (<#6586>) ### Changed - Raised the default guide assets limitations to 30 assets, with a maximum size of 10MB each (<#6575>) - \[SDK\] Custom `ProgressReporter` implementations should now override `start2` instead of `start` The old implementation is still supported. (<#6556>) - Improved memory optimization and code in the decoding module (<#6585>) ### Removed - Removed the YOLOv5 serverless function (<#6618>) ### Fixed - Corrected an issue where the prebuilt FFmpeg bundled in PyAV was being used instead of the custom build. - Fixed the filename for labels in the CamVid format (<#6600>)

There are several problems with how progress reporting is handled in the SDK, both on the interface and implementation level: * The user is supposed to know, for a given function, which units it will report progress in. This is unnecessary coupling and prevents us from switching to different units, or to have several progress bars using different units. * To create a TqdmProgressReporter, you have to create a tqdm instance, which immediately draws a progress bar. This works poorly if the function prints any log messages before the progress actually starts. * There's no easy way to automatically call `finish` on a progress bar, so some functions don't (for example, `Downloader.download_file`). This can cause unexpected output, since tqdm will refresh the progress bar in a background thread (possibly after we've already printed something else). * `iter` is basically broken, because it divides by `period`, which is 0 in all current implementations. * Even ignoring that, it's hard to use correctly, because you need to manually call `finish` in case of an exception. * `split` is not implemented and not used. * `StreamWithProgress.seek` assumes that the progress bar is at 0 at the start, and so does `ProgressReporter.iter`. The former also works incorrectly if the second argument is not `SEEK_SET`. Fix these problems by doing the following: * Add a new `start2` method which accepts more parameters. The default implementation calls `start`, so that if a user has implemented the original interface, it'll keep working. * Add a `DeferredTqdmProgressReporter` that accepts tqdm parameters instead of a tqdm instance, and only creates an instance after `start2` is called. Use it where `TqdmProgressReporter` was used before. The old `TqdmProgressReporter` is kept for compatibility, but it doesn't support any `start2` arguments other than those supported by the original `start`. * Add a `task` context manager, which automatically calls `start2` and `finish`. Use it everywhere instead of explicit `start`/`finish` calls. Remove `start`/`finish` calls from `StreamWithProgress` and `iter`. * Implement basic assertions to ensure that `start2` and `finish` are used correctly. * Remove `period` and `split`. * Rewrite `StreamWithProgress.seek` and `ProgressReporter.iter` to use relative progress reports. These changes should be backwards compatible for users who pass predefined or custom progress reporters into SDK functions. They are not backwards compatible for users who try to use progress reporters directly (e.g. calling `start`/`finish`). I don't consider that a significant issue, since the purpose of the `ProgressReporter` interface is for the user to get progress information from the SDK, not for them to use it in their own code. Originally developed for cvat-ai#6483.

SpecLad requested a review from zhiltsov-max July 24, 2023 18:03

SpecLad requested review from azhavoro and mdacoca as code owners July 24, 2023 18:03

SpecLad force-pushed the progress-reporting-rework branch from f078d7c to e1fa4e5 Compare July 25, 2023 17:03

zhiltsov-max reviewed Jul 26, 2023

View reviewed changes

SpecLad added 4 commits July 27, 2023 19:04

Update changelog

fb18b8b

Be more explicit about what arguments progress reporters must expect

a6408a6

Rename TqdmProgressReporter2

dfe7a94

SpecLad force-pushed the progress-reporting-rework branch from 3c8c1ff to dfe7a94 Compare July 27, 2023 16:12

zhiltsov-max approved these changes Jul 28, 2023

View reviewed changes

SpecLad merged commit 4b86439 into cvat-ai:develop Jul 28, 2023
34 checks passed

SpecLad deleted the progress-reporting-rework branch July 28, 2023 09:09

SpecLad mentioned this pull request Aug 11, 2023

Release 2.6.0 #6660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp the progress reporting API #6556

Revamp the progress reporting API #6556

SpecLad commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading

zhiltsov-max Jul 26, 2023 •

edited

Loading

SpecLad Jul 27, 2023

zhiltsov-max Jul 27, 2023

SpecLad Jul 27, 2023

Revamp the progress reporting API #6556

Revamp the progress reporting API #6556

Conversation

SpecLad commented Jul 24, 2023 • edited Loading

Motivation and context

How has this been tested?

Checklist

License

codecov bot commented Jul 24, 2023 • edited Loading

Codecov Report

zhiltsov-max Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

SpecLad Jul 27, 2023

Choose a reason for hiding this comment

zhiltsov-max Jul 27, 2023

Choose a reason for hiding this comment

SpecLad Jul 27, 2023

Choose a reason for hiding this comment

SpecLad commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading

zhiltsov-max Jul 26, 2023 •

edited

Loading