New modular metric interface

[SkafteNicki]

What does this PR do?

Fixes #3069

This is a proposal on how an extension for the modular interface for
metric packages could look like. What our interface is missing, is the option to do computations
after dpp sync. Consider the following example for rmse:

20 samples, 2 devices, 10 for each devices
With our current setup, each device would return a metric value which then
get synced.

First machine returns (assume sum of squared error is 200)
sqrt(1/10 * 200) = 4.47
Second machine return (assume sum of squared error is 100)
sqrt(1/10 * 100) = 3.16
After ddp sync the value that is returned would be
(4.47 + 3.16) / 2 = 3.815
But the correct value is:
sqrt(1/20 * 300) = 3.872

It is possible to arrive at the correct result if each  machine instead returns 
a *metric state* i.e a collection of values that can be synced and the correct results can
derived from (in above case that would be the number of samples and the
sum of squared errors). Thus, we need to extend the modular interface, such
that we (or the user) can do calculations after the ddp sync.

This PR therefore propose to go from our decorator orientated modular interface
to a hook-based interface. All hooks are optional, such that the user only needs
to implement forward if the inherent from either TensorMetric or NumpyMetric.

Hooks to add:

• input_convert -> used to convert the input to correct type
• output_convert -> used to convert the output of forward
• ddp_sync -> do ddp_sync
• compute -> this is the missing part, that enables us to do computations after ddp sync

Note: this PR just implement the hooks, but I still need to go over each metric,
fixing those where the compute hook is needed.
That will be fixed in follow up PR, since this is already extensive enough.

Tagging @justusschock and @Borda for opinion

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[mergify]

This pull request is now in conflict... :(

[justusschock]

In total I really like those changes!

I feel like we should automatically sync the output of forward whenever being on ddp, and then call aggregate on it. We could also think on defaults (i.e. aggregate could be a simple mean by default).

So the next step would be to revisit all metrics for reduction?

Edit: This should also make it simpler to pickle metrics, since we only need to make sure the converters can be pickled :)

[SkafteNicki]

@justusschock thanks, I took me a while to figure out how to keep this as close to native pytorch as possible, but still expressive enough to support the features we need.
I think the first step is to change the backend to accommodate the features that we need and then do the actual implementation of the features.

For aggregation over multiple batches, one way to achieve this to introduce a new Accumulate class that inserts a hook just before ddp_sync, that accumulate the states:

class Accumulate(nn.Module):
    def __init__(self, base_metric):
        super().__init__()
        self.base_metric = base_metric
        self.base_metric._forward_hooks.popitem(-1) # remove compute hook
        self.base_metric.register_forward_hook(self.save_state) # add save_state hook
        self.base_metric.register_forward_hook(self.base_metric.compute) # re-add compute hook
        self.state = None
    def forward(self, *args, **kwargs):
        return self.base_metric(*args, **kwargs)
    def save_state(self, module, input, output):
        if self.state is None:
            self.state = output
        else:
            state = [s+o for s,o in zip(self.state, output)]
            self.state = state
            output = state
        return output
    def reset(self):
        self.state = None
# Use case
metric = Accumulate(Metric(...))
for (data, target) in dataloader:
    pred = model(data)
    val = metric(pred, target) # val will be the accumulated value

[codecov]

Codecov Report

Merging #2528 into master will decrease coverage by 9%.
The diff coverage is 88%.

@@           Coverage Diff            @@
##           master   #2528     +/-   ##
========================================
- Coverage      90%     81%     -9%     
========================================
  Files          81      84      +3     
  Lines        7858    9321   +1463     
========================================
+ Hits         7034    7518    +484     
- Misses        824    1803    +979     

[justusschock]

@SkafteNicki I see your point. But isn't it basically the same whether you accumulate across nodes or across different batches in the same node? This would probably avoid some code duplication.

We should have a chat in slack considering the perfect integration, once we finished the API for metrics.

[SkafteNicki]

@justusschock it probably is the same syncing between nodes and across the same node, so I agree that it should somewhat be handled in the same way. Only difference is that accumulation on the same node, should probably be a feature that the user can enable/disable (i.e. could be a accumulate flag in the metric __init__).

For now, I will rename the ddp_sync hook to aggregate as you proposed, and it will still just support sync between nodes. Then we can discuss on slack the remaining parts.

[mergify]

This pull request is now in conflict... :(

[mergify]

This pull request is now in conflict... :(

[justusschock]

@SkafteNicki How is it going with this PR?

[mergify]

This pull request is now in conflict... :(

[mergify]

This pull request is now in conflict... :(

[Borda]

LGTM 🐰

[Borda]

@SkafteNicki some metric test seems to be hanging...

[SkafteNicki]

@Borda which test are hanging, I cannot figure it out from drone details?

[Borda]

@Borda which test are hanging, I cannot figure it out from drone details?

see this build - http://35.192.60.23/PyTorchLightning/pytorch-lightning/8999
the las passing tests was tests/metrics/test_metrics.py::test_metric[metric1] PASSED [ 37%] so the next one hanged...

[SkafteNicki]

@Borda fixed the bug, we can merge this now :]