What's purpose of logging?

[vadimkantorov]

🐛 Describe the bug

What is the purpose of logging? Is it going to auto-upload usage statistics somewhere?

Regarding implementation, what is the reason for not just doing it in a separate file or at least with a decorator? Currently it pollutes the library code during code browsing, especially for simple small functions. It also hurts copy-pasteability, as one needs to delete all this logging unnecessary for small copied snippets.

Versions

It would also be nice to make this field not required. Often, there is no specific version. Core PyTorch also doesn't force entering version.

[kazhang]

Hi @vadimkantorov , thanks for reporting! The purpose of logging is that when PyTorch is used in a custom build environment (e.g. Meta), it's useful to track usage of various APIs centrally. For users outside of the organization, this is a no-op. The API was introduced to PyTorch core in pytorch/pytorch#20698 and has been extensively used in core.

what is the reason for not just doing it in a separate file

We're interested in events like model instantiation, that's why we want to trigger the logging in model constructor. We considered adding it to model base class(#4569) but we don't have any other features to be in the base class.

at least with a decorator

Good point, at least the logging code does not show up in the function definition.

[NicolasHug]

[sadly, there is no category for general discussion]
Versions
It would also be nice to make this field not required. Often, there is no specific version. Core PyTorch also doesn't force entering version.

You can use the blank template instead:

Bug reports almost always require a version, and we do this to avoid low quality or low efforts posts. Pytorch core will likely follow soon BTW.

[vadimkantorov]

Maybe it could be added by having in some file

func = add_logging(func)
SomeModule.__init__ = add_logging(SomeModule.__init__)

This way pure functional code rests clean. I guess one problem will be more convoluted debugging experience, but maybe something can be done so that this logging code is skipped by pdb by default

[NicolasHug]

I agree with you both that a decorator could do the job and be less intrusive for classes and functions.

Could be a decent first issue @fmassa @datumbox ?

[fmassa]

I don't have any concerns of adding those loggings in separate files, as long as it doesn't break torchscript.

We could even keep it private to only be logged within fbcode.

[vadimkantorov]

About forcing to report versions, snippets for learning versions are not documented in core: pytorch/pytorch#32864

[kazhang]

Using decorator to log function API usage seems to be a dead end 😩: JIT doesn't like decorator (more specifically **kwargs).

We can apply decorator on models/transforms/datasets but not ops/io. I'm leaning towards using two methods in torchvision: decorator and log API, until we find a better solution (or not going to support TorchScript)

[datumbox]

I agree that ideally a decorator would have been nicer, but I'm not convinced that having 2 separate implementations or sacrificing JIT scriptability is worth it. IMO the current solution is a one-liner and it's clean enough.

[kazhang]

I found a way to inspect caller's module and function name, we could further simplify the log function to be like

_log_api_usage_once()  # without passing the name of function e.g "torchvision.ops.nms"

https://stackoverflow.com/questions/1095543/get-name-of-calling-functions-module-in-python

-EDIT-
NVM, there seems to be some performance tax: https://stackoverflow.com/questions/41481722/python-traceback-performance-problems

[datumbox]

Indeed you can capture the caller without passing parameters but I think we are very likely to still get issues with JIT. Plus the performance issues you noted.

Something that I want to make more crisp is what we try to optimize by this change. Does omitting the string in the function call make the code better or easier to read?

I would argue that since the method call can't be removed, the lack of parameter makes little difference. Maybe passing the string makes it more clear what the logger does. I could be wrong but it feels like to resolve this issue, given our JIT constrains, is to document what the _log_api_usage_once() method does and provide clear details on what it captures and what it doesn't capture. The clarifications posted above sounds enough. Shall we do that on a new PR?

I'm open to more ideas on how to make the code more readable, I just thing it's hard to optimize this case by a lot as it's already a one-liner.