Skip to content

Simplified usage log API #5095

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Dec 16, 2021
Merged

Simplified usage log API #5095

merged 11 commits into from
Dec 16, 2021

Conversation

kazhang
Copy link
Contributor

@kazhang kazhang commented Dec 13, 2021

As we discussed in #5052, we want to:

  1. log fully qualified name;
  2. use a simple API that takes either class self or function as input;

@facebook-github-bot
Copy link

facebook-github-bot commented Dec 14, 2021

💊 CI failures summary and remediations

As of commit 5458cde (more details on the Dr. CI page):


  • 1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job Step Action
CircleCI cmake_windows_cpu set -ex
source packaging/windows/internal/vc_install_helper.sh
packaging/build_cmake.sh
🔁 rerun

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.

@kazhang kazhang reopened this Dec 14, 2021
@kazhang kazhang changed the title [experimental] log API v3 Simplified usage log API Dec 14, 2021
@kazhang kazhang marked this pull request as ready for review December 14, 2021 23:49
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.

Thanks @kazhang. I think the approach works.

I noted a couple of log calls that were missed that I think are worth adding in this PR to wrap it up.
Also I noticed that a couple of tests are failing.

@kazhang
Copy link
Contributor Author

kazhang commented Dec 15, 2021

Once this PR is merged, I will update #5073 and #5007.

@@ -88,6 +89,7 @@ def norm_range(t, value_range):
else:
norm_range(tensor, value_range)

assert isinstance(tensor, torch.Tensor)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type check complaints about

Item "List[Tensor]" of "Union[Tensor, List[Tensor]]" has no attribute "size"

Since list of tensors should have been stacked together on line 57, I think this is a fair assertion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @kazhang, clearly this error is not related to your changes.

@pmeier I tagged you because of the git-blame info. The tensor: Union[torch.Tensor, List[torch.Tensor]] is indeed defined as list. My understanding is that this won't work, can you confirm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, this works as expected:

# if list of tensors, convert to a 4D mini-batch Tensor
if isinstance(tensor, list):
tensor = torch.stack(tensor, dim=0)

I look into it an see why mypy doesn't pick up on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I looked it quickly and missed this. Sounds good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Keeping this open for now. I'm fine with the fix for this PR, but something weird is going on. I can reproduce the issue, but this PR shouldn't do anything to trigger it. I'll investigate more thoroughly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmeier just to clarify, should I remove this assertion and ignore the type check error for now? since you'll investigate more thoroughly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep it in for now. I'll try to figure out how to make mypy happy without the assert in.

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.

Thanks a lot for the work @kazhang. I think this looks much better.

@NicolasHug could you also have a quick look just to be sure?

@vfdev-5 Does it make sense to add log calls to the _interpolate_bilinear2d_aa and _interpolate_bicubic2d_aa methods on a separate PR?

FYI @pmeier We are getting some failures on the dataset failures on the prototype.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 16, 2021

@datumbox I think we can skip logging these methods (_interpolate_bilinear2d_aa, _interpolate_bicubic2d_aa)

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @kazhang , one nit (feel free not to address it) but LGTM anyway!

@kazhang kazhang merged commit eac3dc7 into pytorch:main Dec 16, 2021
@github-actions
Copy link

Hey @kazhang!

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

facebook-github-bot pushed a commit that referenced this pull request Dec 21, 2021
Summary:
* log API v3

* make torchscript happy

* make torchscript happy

* add missing logs to constructor

* log ops C++ API as well

* fix type hint

* check function with isinstance

Reviewed By: prabhat00155

Differential Revision: D33253475

fbshipit-source-id: 32a2d61d1c993cf9456905437d1d7cb3bd467e04

Co-authored-by: Vasilis Vryniotis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants