[BC-breaking] Make Metrics accumulate values on device specified by user (#1232)

[vfdev-5]

• update accuracy to accumulate _num_correct in a tensor on the right device

• update loss metric to accumulate _sum in a tensor on the right device

• update mae metric to accumulate in a tensor on the right device

• update mpd metric to accumulate in a tensor on the right device

• update mse metric to accumulate in a tensor on the right device

• update top k accuracy metric to accumulate in a tensor on the right device

• update precision and recall metrics to accumulate in tensors on the right device

• black formatting

• reverted run*.sh

• change all metrics default device to cpu except running_average

• Update ignite/metrics/precision.py

• remove Optional type from metric devices since default is cpu

• add comment explaining lack of detach in accuracy metrics

Fixes #1082

Original PR #1232

Description:

• Merge PR to master

Check list:

• New tests are added (if a new feature is added)
• New doc strings: description and/or example code are in RST format
• Documentation is updated (if required)

cc @n2cholas

[vfdev-5]

@n2cholas now I think we have to update the docstrings and main docs on metrics and how to use the device etc...
Please, send other PR to this branch from your fork such that we could update everything here and finally merge to master once we are done.

EDIT: I have to also check the code on real TPUs and run some local tests...

[vfdev-5]

Seems like some tests on Horovod are failing: https://app.circleci.com/pipelines/github/pytorch/ignite/618/workflows/c74dff3a-5840-418e-837f-c43c4ac4b646/jobs/1308

I can inspect it later.

[n2cholas]

@vfdev-5 The new tests I wrote checking that the accumulators are on the right device are failing. It's very strange that the other two_gpu tests are passing and only horovod is failing. Is it possible to run horovod on CPU only (like it was for TPUs)? If so, I can take a stab at it.

[vfdev-5]

We compile Horovod from source on the CI : https://github.com/pytorch/ignite/blob/master/.circleci/config.yml#L234
You can try either simply install it from pip, so probably it will be only basic CPU version. Either compile it as in our CI without Nccl support. To run horovod on CPU only, you can use CUDA_VISIBLE_DEVICES="" pytest ....

[n2cholas]

@vfdev-5 I was able to set horovod up and run the distributed tests on the CPU but could not reproduce the error from CI (the tests pass for me). Would you mind taking a look, when you have a chance?

[vfdev-5]

@n2cholas thanks for the info ! Yes, I'll try to take a look as soon as possible.

[vfdev-5]

@n2cholas just checked one test pytest tests/ignite/metrics/test_accuracy.py::test_distrib_hvd on 1 GPU with Horovod installed with NCCL support and I could reproduce the error:

pytest tests/ignite/metrics/test_accuracy.py::test_distrib_hvd

And I confirm that running the test on CPU is passing.

The problem more precisely is about

Mon Aug 17 08:15:55 2020[0]<stderr>:    assert acc._num_correct.device == device, "{} vs {}".format(acc._num_correct.device, device)                                        
Mon Aug 17 08:15:55 2020[0]<stderr>:AssertionError: cuda:0 vs cuda 

[vfdev-5]

@n2cholas I checked the tests of accuracy and I proposed a way to improve them and also fix the issue:
b8a4355
Could you please adapt all other tests like that. Thanks !

EDIT: shouldn't we update RunningAverage metric, at least handle device properly ?

[n2cholas]

@vfdev-5 Thanks for the improved tests, I'll update the rest soon.

For the RunningAverage metric, I updated the metric to use src's device when src is a Metric, and otherwise use the device parameter. However, this will only accumulate on the desired device when the input is a tensor. You will see what I mean if you take a look at 90b5b85. Please let me know if you think the behaviour should be different.

Since I'm committing to my metrics_impl branch, this PR and the docs PR seem to be getting mixed up. Should I be using two different branches? I was not able to commit directly to this branch (ignite/metrics_impl)

[vfdev-5]

For the RunningAverage metric, I updated the metric to use src's device when src is a Metric, and otherwise use the device parameter. However, this will only accumulate on the desired device when the input is a tensor. You will see what I mean if you take a look at 90b5b85. Please let me know if you think the behaviour should be different.

OK, I understand. Maybe we should check RunningAverage.src.device vs argument input device and raise an error if different or either override RunningAverage.src.device... What would be more helpful for the user ?

Since I'm committing to my metrics_impl branch, this PR and the docs PR seem to be getting mixed up. Should I be using two different branches?

I will merge #1239 once we addressed all visible issues and it will be in pytorch/metrics_impl branch. I know that this is not an ideal workflow but for instance it is an eco compromise between running gpu tests on all PRs and only on project's branches...

[n2cholas]

OK, I understand. Maybe we should check RunningAverage.src.device vs argument input device and raise an error if different or either override RunningAverage.src.device... What would be more helpful for the user ?

That makes sense to me. So, by default it will be None, and we will use RunningAverage.src._device in this case. If the user passes an actual device in we check to make sure it's the same as the device for src. I'll fix this and write the tests.

I will merge #1239 once we addressed all visible issues and it will be in pytorch/metrics_impl branch. I know that this is not an ideal workflow but for instance it is an eco compromise between running gpu tests on all PRs and only on project's branches...

Got it, thanks!

[vfdev-5]

That makes sense to me. So, by default it will be None, and we will use RunningAverage.src._device in this case. If the user passes an actual device in we check to make sure it's the same as the device for src. I'll fix this and write the tests.

Maybe None wont work if we use output instead of metric as source... It would be better probably to keep the same init as for other metrics

[n2cholas]

Maybe None wont work if we use output instead of metric as source... It would be better probably to keep the same init as for other metrics

In the current logic, the init has None as the default, then if an output is used instead of a metric and the user doesn't specify a device, we use torch.device("cpu").

My only concern with using CPU as the default argument is if the user passes in a metric on the GPU, they will have to specify the same device as an argument to RunningAverage, otherwise we throw an error. With the current implementation, we can check if device is None and then use the same device as the specified metric automatically. Then, we only throw an error when the user explicitly passes in the wrong device for running average.

Also, I would like to clarify one thing: we only use the specified device for the accumulator when the output/metric.compute produces a tensor (since when it is just a python scalar, we don't want to re-wrap it in a tensor). Is this behaviour acceptable? If so, I already made a note for this in the docstring. Should I clarify this anywhere else?

[vfdev-5]

In the current logic, the init has None as the default, then if an output is used instead of a metric and the user doesn't specify a device, we use torch.device("cpu").
My only concern with using CPU as the default argument is if the user passes in a metric on the GPU, they will have to specify the same device as an argument to RunningAverage, otherwise we throw an error. With the current implementation, we can check if device is None and then use the same device as the specified metric automatically. Then, we only throw an error when the user explicitly passes in the wrong device for running average.

Yes, you are right. Sounds good to me.

Also, I would like to clarify one thing: we only use the specified device for the accumulator when the output/metric.compute produces a tensor (since when it is just a python scalar, we don't want to re-wrap it in a tensor). Is this behaviour acceptable? If so, I already made a note for this in the docstring. Should I clarify this anywhere else?

Do we have a cases when updated output produces a scalar and not tensor ? Maybe I didn't understand your point about "output/metric.compute produces a tensor"...
I agree that it does not make much sense to wrap counters like self._num_examples etc.

Should I clarify this anywhere else?

If this could help user to better understand what happens under the hood, I'd say yes. Well, let me see the complete doc and we can decide later if we need to add something specific...

[n2cholas]

Do we have a cases when updated output produces a scalar and not tensor ? Maybe I didn't understand your point about "output/metric.compute produces a tensor"...
I agree that it does not make much sense to wrap counters like self._num_examples etc.

I believe most of the metrics's compute method produces a float instead of a tensor. For example, we can look at MeanSquaredError.compute(), which returns a float. So, if we use RunningAverage(MeanSquaredError()), every time we call compute, we have the following logic:

    def compute(self) -> Union[torch.Tensor, float]:
        if self._value is None:
            self._value = self._get_src_value()
        else:
            self._value = self._value * self.alpha + (1.0 - self.alpha) * self._get_src_value()

        return self._value

We call MSE's compute via RunningAverage._get_src_value(), which returns a float, therefore self._value will be a float instead of being a tensor on the device the user specified.

I think this is fine, but I want to make sure it's what we want.

[vfdev-5]

Ah OK, I understand now. Thanks for the explanation. Yes, me too, I think this is fine.

[vfdev-5]

@n2cholas if you could add distritbuted tests that reproduce the problem from #1242 it would be awesome (at least for problem 1 and 3 as you say that they can be fixed)

[vfdev-5]

@fco-dv @sdesrozis please start to review

[n2cholas]

Seeing the colab, this corresponds to single TPU execution and not CPU. If NUM_TPU_WORKERS=8 is set we run on 8 devices in distrib mode, otherwise it picks a single TPU and run on it.

Thanks for catching this!

I propose to create a script to measure execution times, a sort of integration test with Engine and metrics:

• in a single test, let's generate random y_pred, y to compute metrics on for 100 iterations, 3 epochs.

• reimplement metrics like Accuracy and Accumulation to keep current master implementation (let's call it "as number" implementation vs this PR's "as tensor" implementation)

• run benchmarks per metric on both implementations

  • let's run 3-5 time each test for better time stats

  • in various confs:

    • only on CPU, single device, no distributed
    • only on GPU, single device, no distributed
    • only on GPUs, two devices, distributed
    • only on TPU, single device, no distributed
    • only on TPUs, 8 devices, distributed

What do you think ?

This sounds good. I'll work on this soon.

I wonder what would be better to do:

• detach tensors in output of update(output) inside iteration_completed
  or
• explicitly in each update function.

In the first case, as we do not restrict the API and user can use either attach method or update/compute methods. If update/compute methods are used, that it is up to user to detach things before calling update or use torch.no_grad() context manager. Is it acceptable as requirement ?

I would prefer to detach explicitly in each update function so the user doesn't have to think about it, as in the current master users don't need to worry about detaching. But I'm not too familiar with with how the metrics are used in the engine (I use ignite solely for the metrics), so I will defer to your judgement on this decision.

I addressed the comments in my branch (n2cholas/ignite:metrics_impl), and am not sure how to push those commits into this branch. Should I open a separate pull request...?

[vfdev-5]

I would prefer to detach explicitly in each update function so the user doesn't have to think about it, as in the current master users don't need to worry about detaching. But I'm not too familiar with with how the metrics are used in the engine (I use ignite solely for the metrics), so I will defer to your judgement on this decision.

Metrics reset-update-compute API can be introduced as

acc = Accuracy()

acc.reset()

with torch.no_grad():
    acc.update((y_pred1, y1))

with torch.no_grad():
    acc.update((y_pred2, y2))

print(acc.compute())

In this case, detach is not needed in the update. Otherwise, we have ensure everywhere that we detached y_pred and y. This is just 2 additional lines of code, not a big deal though.

But I'm not too familiar with with how the metrics are used in the engine (I use ignite solely for the metrics)

@n2cholas try it with the Engine :)

I addressed the comments in my branch (n2cholas/ignite:metrics_impl), and am not sure how to push those commits into this branch. Should I open a separate pull request...?

Well, technically, yes, you have to send PRs to this branch. But if you plan to work more this week, exceptionally I can give you a temporary write permission to push directly to this branch.

[n2cholas]

Metrics reset-update-compute API can be introduced as

acc = Accuracy()

acc.reset()

with torch.no_grad():
    acc.update((y_pred1, y1))

with torch.no_grad():
    acc.update((y_pred2, y2))

print(acc.compute())

In this case, detach is not needed in the update. Otherwise, we have ensure everywhere that we detached y_pred and y. This is just 2 additional lines of code, not a big deal though.

I see! What about changing the update method so it has the torch.no_grad() decorator, as follows:

@torch.no_grad()
@reinit__is_reduced
def update(self, output):
    ...

I'm just wondering if there's a good reason to push this overhead to the user. Is there any case where the user would not want the tensors to be deatched?

@n2cholas try it with the Engine :)

Haha will do on my next PyTorch project!

Well, technically, yes, you have to send PRs to this branch. But if you plan to work more this week, exceptionally I can give you a temporary write permission to push directly to this branch.

No worries, I was just curious. I will work on the benchmarks then create a PR as usual.

[n2cholas]

I've run the integration benchmarks on CPU, GPU, and multi-GPU. Here is a gist with the code and here are the numbers I got (single gpu tests are on titan V, multi-gpu on titan V + titan RTX). The error represents the margin of error for a 95% confidence interval (you can see how it's generated in the gist). Overall, the results are what we expect: the new implementation is slightly slower for CPU workloads, but slightly faster for GPU and distributed workloads.

I was trying to see what was going on with XLA, and I think for now we should not allow users to keep metrics on an XLA device, and instead they should use the metrics on the normal CPU when training on the TPU. We can throw an error if they try to set the metric's device to an XLA device. I'm not sure how to fix the performance issues we were seeing. Does this sound good @vfdev-5?

[vfdev-5]

@n2cholas thanks a lot for the benchmark ! It is very helpful to see complete picture of that !

I was trying to see what was going on with XLA, and I think for now we should not allow users to keep metrics on an XLA device, and instead they should use the metrics on the normal CPU when training on the TPU. We can throw an error if they try to set the metric's device to an XLA device. I'm not sure how to fix the performance issues we were seeing. Does this sound good @vfdev-5?

I agree that we can use CPU instead of XLA for now, before finding out where the problem and if it can be solved somehow...
Probably, the issue is that XLA compiler should create a graph for all those updates and thus it takes time...

[vfdev-5]

@n2cholas I left few comments. I still have the question about explicit detach calls. Now we have some classes where we call it directly on the output and other where we are not calling it at all... I think it would be better to make everything in the same way : call detach on the output. What do you think ?

EDIT: There some failing horovod tests on gpu to fix : https://app.circleci.com/jobs/github/pytorch/ignite/1477

[vfdev-5]

Shouldn't we detach output if it is a tensor here ? To have something like

if self._device is not None:
    # Put output to the metric's device
    if isinstance(output, torch.Tensor):
        output = output.detach()
        if (output.device != self._device):
            output = output.to(self._device)

self.accumulator = self._op(self.accumulator, output)

[n2cholas]

Yes, we should, thanks for catching this.

[vfdev-5]

Let's remove this comment and call detach on the output as in other classes:

# y_pred, y = output
y_pred, y = output[0].detach(), output[1].detach()

[n2cholas]

Sounds good!

[n2cholas]

@vfdev-5 I agree, I'll ensure all the metrics consistently call detach on the outputs (and fix those horovod tests too).

[vfdev-5]

@n2cholas thanks a lot for working on this huge PR !
I'll fix the CI failing on TPUs and we can merge it

[vfdev-5]

@n2cholas we are preparing a v0.4.2 release and I think as being BC-breaking change this commit will be present since v0.5.0.

[n2cholas]

@vfdev-5 Sounds good, thank you!