torchmetrics: Don't import all modules from torchmetrics root

🚀 Feature

@SkafteNicki could we not import all metrics at the root module? https://github.com/PyTorchLightning/metrics/blob/master/torchmetrics/__init__.py

Motivation

Reasons against this:

This slows down import times
Not all requirements might be installed. For instance, torch-fidelity is required for FID under torchmetrics.image. But torchmetrics itself doesn’t require these dependencies to be installed.

Pitch

Keep metric implementation imports at the submodule level, and ensure the root torchmetrics is used for only the essential classes (e.g. Metric)

Alternatives

Additional context

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 17 (16 by maintainers)

Most upvoted comments

still don’t see why top level imports could be problem if there is only torch dependency…

As @Borda mentioned, if there’re no optional packages installed, loading time doesn’t change, but if there are all of them (in a rare situation maybe?), it’s around 4x longer than #463 if I ran a benchmark correctly in the notebook below.

With no optional dependencies
- master: 916 ms
- #463: 912 ms
With all optional dependencies
- master: 4.02 s
- #463: 914 ms

best of 5 runs of import torchmetrics each

Notebook: https://colab.research.google.com/drive/1PY2Nmba8fpxHpyAPAwLpaCdbRrO-kejj?usp=sharing

akihironitta on Aug 24, 2021

For comparison: sklearn has also all metrics importat at the top level at sklearn.metrics. They also rely on numpy like we do on pytorch. On the other hand, as the inventory of metrics grow, it is possible that there can be collisions in different domains where metrics have the same name but are defined very differently. This would be solved by grouping under submodules.

awaelchli on Aug 20, 2021

@ananthsub so how about if we keep in the package root Metric which is truly dependent only on PT and all the needs other/external dependencies will be imported from its sub-packages? Pls understand that otherwise, this is a significant break change with any back compatibility and any warning…

@Borda in order to unblock ourselves and be able to use the latest metrics updates, this seems like the viable solution now which limits the number of breaking changes made. i think we could pursue this, and then revisit the broader import pattern separately. what do you think?

ananthsub on Aug 27, 2021

I would align with Ananth there.

As the project grows, I believe there is need to provide better organizations and namespace, especially it saves loading time.

A benefit from this approach might be to incentivise users to check out other namespace and discover new metrics.

Best, T.C

tchaton on Aug 23, 2021

I think this is worth it considering the vast majority of users import 1-3 metrics at most. Loading the entire package for this is overkill

carmocca on Aug 20, 2021