Refactor: Rename update and compute methods to _update and _compute

[SkafteNicki]

What does this PR do?

Currently we are wrapping users implementation of update and compute. This is in general bad practise and we have received a couple of questions about this (especially because it can be confusing to debug what is going on for the user). Instead we will now ask the user to implement the private methods _update and _compute which will then be called by their public counter-part. The end user will still just call update and compute and the change is therefore backward compatible for users that just uses TM. Users that have implemented custom metrics will need to update their metrics.

I have borrowed the is_overridden function from PL such that we are going to check if metric are overwriting the update and compute method.

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 to update the docs?
• Did you write any new necessary tests?

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 🙃

[Borda]

Also, update docs and how to implement own metric...

[codecov]

Codecov Report

Merging #840 (529bcdc) into master (ffe824a) will decrease coverage by 0%.
The diff coverage is 96%.

@@          Coverage Diff          @@
##           master   #840   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         167    167           
  Lines        6931   6958   +27     
=====================================
+ Hits         6587   6599   +12     
- Misses        344    359   +15     

[SkafteNicki]

@Borda this class in PL now breaks:
https://github.com/PyTorchLightning/pytorch-lightning/blob/8394770d4afa5480f881229b150ac44eaa8c41b0/pytorch_lightning/trainer/connectors/logger_connector/result.py#L201
What should we do?

[Borda]

this class in PL now breaks:
https://github.com/PyTorchLightning/pytorch-lightning/blob/8394770d4afa5480f881229b150ac44eaa8c41b0/pytorch_lightning/trainer/connectors/logger_connector/result.py#L201
What should we do?

@carmocca mind help here...

[SkafteNicki]

So this breaks lightning just as it would break any user implementation outside of torchmetrics.

Yes. We did a vote in core-metrics on slack and it was decided the renaming should be _update and _compute.

Is there a plan for backwards compatibility?

Not really. We want to push breaking API changes now, as we are moving closer to v1.0 release where we cannot make these kind of changes. Do you think we should just stick with what we have and not do the breaking change?

Lightning will need to add support for both versions during a period of time if we don't want to increase the minimum torchmetrics version. We currently require: https://github.com/PyTorchLightning/pytorch-lightning/blob/2d043857eef55eb327528b70c8b33c680e82fb7b/requirements.txt#L9

Yes. We already increased integration with lightning to v1.5 in this release:
https://github.com/PyTorchLightning/metrics/blob/76646b65a0a163d76b138025bb2d4e4a0fe26452/requirements/integrate.txt#L1

[carmocca]

Do you think we should just stick with what we have and not do the breaking change?

It's up to the metrics core team, it's a big breaking change but I agree that it's either now or never.

So if torchmetrics is going to be "moving fast and breaking things" it's probably worth pinning the torchmetrics dependency to 0.8 in PL.

Just beware that this could leave PL users unable to upgrade PL without also upgrading their torchmetrics implementations, so if you don't provide backwards compatibility at least provide clear upgrade guidance for existing Metrics implementations.

Now, we are in a "chicken and egg" situation where both libraries require the other one to release. We would need to orchestrate this before the PL 1.6 release.

One solution would be:

• Disable PL integration testing here.
• Merge this PR
• Prepare update PR in PL to the new API, pinning torchmetrics>=0.8
• Release torchmetrics 1.8
• ASAP merge the PL PR as CI will be failing otherwise.
• Release PL 1.6
• Enable PL integration testing here.

But it's pretty risky. Ideally either torchmetrics keeps backwards compatibility for one version or PL adds support for both until 1.6 is released and then drops the previous format for 1.7

[justusschock]

@carmocca I think for PL itself this should not be a problem, as the compute and update interface stays the same.

I think since the torchmetrics package aims to be independent of PL and PL has it as a requirement, in general we should not keep things, just because they might break PL.

On the other side, this wouldn't be only PL breaking, but every user code, so I suggest we do something about backwards compatibility, despite not being stable yet. My proposal:

For user-backwards compatibility, we could probably check if the update and compute functions are overridden, then do the current behavior and raise a warning, Otherwise just default to the new function.

[SkafteNicki]

@Borda @justusschock @carmocca I updated the PR so the change is now backwards compatible, thus the integrations tests will pass but the user will get a deprecation warning asking them to rename their methods. This is the relevant code:

# check update and compute format
if is_overridden("update", self, Metric):
    warnings.warn(
        "We detected that you have overwritten the ``update`` method, which was the API"
        " for torchmetrics v0.7 and below. Insted implement the ``_update`` method."
        " (exact same as before just with a ``_`` infront to make the implementation private)"
        " Implementing `update` directly was deprecated in v0.8 and will be removed in v0.9.",
        DeprecationWarning,
    )
    self._update_signature = inspect.signature(self.update)
    self.update: Callable = self._wrap_update(self.update)  # type: ignore
else:
    if not hasattr(self, "_update"):
        raise NotImplementedError("Expected method `_update` to be implemented in subclass.")
    self._update_signature = inspect.signature(self._update)

if is_overridden("compute", self, Metric):
    warnings.warn(
        "We detected that you have overwritten the ``compute`` method, which was the API"
        " for torchmetrics v0.7 and below. Insted implement the ``_compute`` method."
        " (exact same as before just with a ``_`` infront to make the implementation private)"
        " Implementing `compute` directly was deprecated in v0.8 and will be removed in v0.9.",
        DeprecationWarning,
    )
    self.compute: Callable = self._wrap_compute(self.compute)  # type: ignore
else:
    if not hasattr(self, "_compute"):
        raise NotImplementedError("Expected method `_compute` to be implemented in subclass.")

We still need to sync with PL, but it will not be as critical.

@carmocca I kept the naming as _update and _compute because that what the majority of the core metrics team wanted.

[carmocca]

Sounds good to me. Do you want to open a PR with the update to PL?

[Borda]

We still need to sync with PL, but it will not be as critical.

Cool, still let's do it before v1.6 is out :)

[williamFalcon]

hey everyone, i just want to clarify the process for any proposed API changes.

• an API change is proposed by anyone (always welcomed of course)
• the API proposal is reviewed by either @awaelchli or @williamFalcon
• we go back and forth
• if the API change is approved, then we can make the change.
• otherwise we close the PR.

the APIs are like the web front-end for everything we do... front-ends are designed by designers (not by a collective effort, otherwise you build a frankenstein). Treat the APIs with the same level of riguour.

What is covered under API?
any key methods, signatures, etc in all the key user-facing modules which include (but are not limited to):

• datamodule
• lightningmodule
• trainer
• Metric

it's the "high-level", "root" classes that a user would mess with directly.

the API does NOT include internal things like:

• plugin API
• etc...

if you're unclear whether it's covered or not, please ask @awaelchli or @williamFalcon

Thanks!