Skip to content

Conversation

fmassa
Copy link
Member

@fmassa fmassa commented Oct 29, 2021

This PR adds logging to the public ops in torchvision.

I haven't tried making it use inspect to automatically find the name of the function as there is no chance this is supported in torchscript.
This function isn't supported in torchscript anyway, so let's make it a no-op in torchscript for now.

A quick benchmark shows ~900ns of overhead, which should be fine

%timeit torchvision.utils._log_api_usage_once('testing')
914 ns ± 17.6 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 29, 2021

💊 CI failures summary and remediations

As of commit 693ea25 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@fmassa
Copy link
Member Author

fmassa commented Oct 29, 2021

Great, need to fix lint :-)

@fmassa
Copy link
Member Author

fmassa commented Oct 29, 2021

Ok, I'll have to fight with torchscript then :-)

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

We could consider adding also on LastLevelMaxPool, LastLevelP6P7, ExtraFPNBlock but it's also OK without it. Can be merged once a scriptable operator for the logging is added as discussed offline.

@oke-aditya
Copy link
Contributor

oke-aditya commented Oct 29, 2021

May I ask, why are we logging functions? I see such addition in many PRs by Francisco.

(If the reason is confidential to FB, please ignore)

Edit
Reason why I wanted to know is whether this some software development practice?

@fmassa
Copy link
Member Author

fmassa commented Oct 29, 2021

Hi @oke-aditya

This function provides similar functionality as logging from the Python stdlib.

We log which methods are called for debugging purposes and it's used only in FB infra.

By no means this collects any data from our open-source users, and is pretty-much a no-op for external users.

This PR in PyTorch introduced this logging functionality, and has more information on the lower-levels of it pytorch/pytorch#20745


def _log_api_usage_once(obj: object) -> None:
torch._C._log_api_usage_once(f"{obj.__module__}.{obj.__class__.__name__}")
@torch.jit.ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @torch.jit.unused
and using
if not torch.jit.is_scripting():

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm changing it now to use torch.jit.is_scripting() or torch.jit.is_tracing()

@oke-aditya
Copy link
Contributor

Thank you for the information :)

My additional doubt was if this would cause additional overhead. But benchmarks indicate it does not affect (as per the PR you linked).

@NicolasHug
Copy link
Member

it's used only in FB infra.
By no means this collects any data from our open-source users, and is pretty-much a no-op for external users.

Maybe we should make this crystal clear in the docstring of _log_api_usage_once, considering fb Meta's press coverage...

@fmassa fmassa merged commit 48ebc0b into pytorch:main Oct 29, 2021
@fmassa fmassa deleted the logging-ops branch October 29, 2021 22:24
@github-actions
Copy link

Hey @fmassa!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

@NicolasHug NicolasHug added module: ops other if you have no clue or if you will manually handle the PR in the release notes labels Oct 29, 2021
facebook-github-bot pushed a commit that referenced this pull request Nov 1, 2021
Summary:
* Add logging to torchvision ops

* Hack to make torchscript work

* Bugfix

* Bugfix

* Lint

* mypy... let's silence it

* Fighting with mymy

* One more try

Reviewed By: datumbox

Differential Revision: D32064710

fbshipit-source-id: a8f46074acba8fcc4cd06efc155f670faa737bba
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
* Add logging to torchvision ops

* Hack to make torchscript work

* Bugfix

* Bugfix

* Lint

* mypy... let's silence it

* Fighting with mymy

* One more try
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/default cla signed module: ops other if you have no clue or if you will manually handle the PR in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants