Add progress and error reporting API

[zhiltsov-max]

Summary

Closes #142

• Added progress and error reporting in Dataset.import_from and Dataset.export (backward compatible)
• Added an ability to skip or fail on import and export problems with specific images or annotations
• Updated COCO, VOC and YOLO formats
• Improved error messages in formats
• Now, Extractor ctor has only kw args. Subclasses must define their own c-tor, which accepts 1 positional and any kwargs. Unused kwargs must be passed to the base class.
• Added Scope.add_many() to enter multiple CMs in 1 call.

How to test

Checklist

• I submit my changes into the develop branch
• I have added description of my changes into CHANGELOG
• I have updated the documentation accordingly
• I have added tests to cover my changes
• I have linked related issues

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.
• I have updated the license header for each file (see an example below)

# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

[IRDonch]

Very limited review so far - I will come back to this.

[IRDonch]

I don't think this passes the type check, because img_id might not be a string at this point.

[zhiltsov-max]

Actually, we might even don't have image id here, but it would be better to include anything for troubleshooting. I need to think more how to report it.

[IRDonch]

IMO, the way that multiple progress bars are handled needs to be reworked. The current approach has two problems:

1. There is no way for the client to gauge how much progress has been made in total, because the caller doesn't know how many times the service (by which I mean the component reporting the progress, such as an extractor) will call start. This will be a problem if Datumaro is integrated into a bigger pipeline which only allows reporting progress as a single fraction, such as Accuracy Checker.

2. It's not possible to have multiple concurrently-updating progress bars, because the report_status and finish functions don't have enough information to determine which bar is being updated.

I propose the following redesign that fixes these problems:

• Remove the desc argument from start and iter.

• Add the following method:

  def split(self, subtasks: Sequence[str]) -> Tuple[ProgressReporter, ...]: ...

  This method tells the reporter that the current task consists of several subtasks (each element of subtasks is the description of a subtask) and returns one progress reporter for each subtask.

• Stipulate that the service must call one and only one of the following methods on its reporter:

  • start - if it can subdivide its work into N homogenous units. If it calls start, then it must use report_status to report on the completion of these units.
  • split - if it can subdivide its work into heterogenous units (subtasks). If it calls split, then no more methods on the original reporter should be called. Instead, the returned reporters must be used by following the rules recursively.

[zhiltsov-max]

Yeah, it makes sense. It still can be hard to solve the first problem with the proposed design. The second problem looks mostly theoretical at this point.

[IRDonch]

It still can be hard to solve the first problem with the proposed design.

What do you mean? It seems fairly easy to calculate:

• If the service has called start, then the progress is the current number of completed units divided by the total number of units (if the service didn't specify the total number, then the progress can only be approximated as 0 before the call to finish and 1 after).
• If the service has called split, then the progress is the sum of the progresses of the child reporters, divided by the number of child reporters.
• If the service has called neither, the progress is 0.

[zhiltsov-max]

Yes, it is basically correct, but the problem is that if we only have a single pbar, children pbars do one of the following:

• Commit to the single pbar. Depending if we know total or not, there are mixed variants possible. For instance, after starting a new pbar, we can get a new total and resize the pbar, or we can get the situation that we overfill the previous pbar (if we knew total, but now we don't). If we know all totals, we can sum them, of course, but it won't mean that units are equal.
• Split pbar into segments and fill them independently
• Rewrite pbar

I don't consider parallel processing here, but it can be a problem, in theory. Generally, I like the idea, but I'm just saying it's not a silver bullet.

[zhiltsov-max]

I've implemented this idea.

[IRDonch]

Okay, but what about about my proposal about moving the description argument to split? I think that makes more sense, because if the service doesn't call split, then it doesn't need to supply a description - the client already knows what it's going to be doing. For example, if the client calls a converter, then the client can always label the progress bar "Exporting dataset", because that's what converters do.

On the other hand, descriptions are necessary when you're splitting the task into subtasks, since the client doesn't know what each subtask represents.

[zhiltsov-max]

Probably, it's reasonable to always split the input pbar, so the caller title is not going to be used at all. I don't have enough arguments for or against requiring titles in split, but it looks more flexible to just ask the number.

[IRDonch]

This class seems useless. It's only used once and in that instance it could just be replaced by ImportContext.

[zhiltsov-max]

From my point, it clearly designates the caller intention, while provision of defaults in the base class is more of a forward compatibility.

[IRDonch]

I don't really understand the nuance here. Is there any circumstance where ImportContext() would act any differently from NullImportContext()? You're just giving the user a pointless choice.

In addition, this enables users to write nonsense like NullImportContext(error_policy=...), which seems undesirable.

[zhiltsov-max]

I don't really understand the nuance here. Is there any circumstance where ImportContext() would act any differently from NullImportContext()? You're just giving the user a pointless choice.

I see it that way that ImportContext is just a base class. It doesn't have to be usable, though it can provide convenient defaults.

In addition, this enables users to write nonsense like NullImportContext(error_policy=...), which seems undesirable.

Why they would need to do this?

[IRDonch]

Why is this a TODO? It seems to already be done.

[zhiltsov-max]

The idea is that laziness is not an input parameter in the current implementation, and the extractors are not aligned on this topic. Ideally, it should be a function parameter and should also be aligned with the eager_mode CM.

[IRDonch]

Why is _ImportFail necessary? What if this function just raised error?

[zhiltsov-max]

It is used to indicate exit from importing process. Other exceptions can be captured by other methods of this class.

[IRDonch]

Other exceptions can be captured by other methods of this class.

Isn't that a good thing, though? Imagine that you want to ignore items that failed to be imported for any reason (including bad annotations). You could write:

    def _handle_item_error(self, error: ItemImportError):
        pass

    def _handle_annotation_error(self, error: AnnotationImportError):
        ???

But if _handle_annotation_error calls fail, then the whole import fails; while if it does nothing, the annotation is ignored, and the item is still used. It seems like it would be better if _handle_item_error got a shot at handling the error raised by _handle_annotation_error.

[zhiltsov-max]

This is a good question. The behavior is going to be quite inconsistent, because we can have any of the following cases in different formats:

[try] load item()
    [try] load annotations()

[try] load item()
[try] load annotations()

[try] load item and annotations()

And if we have eg. handle_ann -> fail, handle_item -> skip, then in the second case we'll fail, then in the first and the third we will skip. If we don't capture errors, we can also do any of them. If we assume the format is "right" and allows this, then yes. Probably, we can just throw error, but then also allow to capture the error in the report_item_error. We still need to wrap errors in dedicated classes to indicate their kinds.

[IRDonch]

It would be more readable to assign each bar to its own variable.

[IRDonch]

Unnecessary default argument?

[IRDonch]

Actually, there's a bigger problem here - a generator function shouldn't be scoped, since then the closing becomes nondeterministic.

[zhiltsov-max]

It just suppresses the linter message, because iter cannot have arguments. But we need an argument here (unless we go for code generation), because otherwise it is not being captured by a generator (which appears from yield) (see #661).

Actually, there's a bigger problem here - a generator function shouldn't be scoped, since then the closing becomes nondeterministic.

It is one of the reasons why error and progress reporting cannot be used in lazy mode.

[IRDonch]

Seems to me like another reason why the progress reporter should not be a context manager.

[zhiltsov-max]

After experimenting and thinking over, I decided to remove closing from plugins, but restore finish. In the end of the day, it feels natural for pbar (and ui, possible) to manage its children objects. Finishing is still important for signaling the finish of the loading in a pbar. In case of error, we aren't going to have any new pbars. There is still a question if we need a dedicated close (to close an unfinished pbar), but it is not that important now.

[IRDonch]

Perhaps the failing behavior should just be the default? That way, a custom policy that wants to only ignore a certain kind of error will only need to override one method.

It'll also allow other kinds of errors to be added in the future without breaking compatibility.

[zhiltsov-max]

Yes, it makes sense. Done

[IRDonch]

I don't think you pushed it.

[zhiltsov-max]

No, it's in
Make failing the default action
8736ea2

[zhiltsov-max]

I'll merge it. There's definitely a lot of improvements can be done, but they are not critical and can be done later.