Skip to content

add api usage log for io #5008

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 24 commits into from
Closed

add api usage log for io #5008

wants to merge 24 commits into from

Conversation

kazhang
Copy link
Contributor

@kazhang kazhang commented Nov 29, 2021 •
edited
Loading

Log IO api usages.
Partially-close: #4955

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 29, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit c78335e (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.

@kazhang kazhang requested a review from fmassa November 29, 2021 23:43
@kazhang kazhang marked this pull request as ready for review November 30, 2021 07:06
@pmeier @NicolasHug
cleanup CI config (#4983)
65cdaea 
* [DIRTY] cleanup CI config

* remove pip install command

* fix syntax

* fix jobs

* fix syntax

* add support for editable install

* sync

* fix sudo apt install

* fix editable install

* sync

* try pip without user install

* [DIRTY] address review comments

* [DIRTY] try use dynamic name

* switch logic

* address remaining comments

* cleanup

* more cleanup

* fix enabling prototype tests

* Update .circleci/config.yml.in

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

* linebreak

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
datumbox
datumbox reviewed Nov 30, 2021
View reviewed changes
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 for the PR @kazhang.

The added logging looks good. Some of the calls (for example read_image()) will generate additional logging as they call other public methods but I don't think this is an issue or something we can avoid.

There is at least one more logging needed at the constructor of VideoReader defined here.

Finally, does it make sense to consider at this point adding the logging calls on the C++ side so that we can include cases where the Python interface is not used? I think that has been discussed offline at one point.

datumbox and others added 16 commits November 30, 2021 12:56
@datumbox
Refactor the get_weights API (#5006)
3d8723d 
* Change the `default` weights mechanism to sue Enum aliases.

* Change `get_weights` to work with full Enum names and make it public.

* Applying improvements from code review.
@pmeier
improve COCO prototype (#4650)
39cf02a 
* improve COCO prototype

* test 2017 annotations

* add option to include captions

* fix categories and add tests

* cleanup

* add correct image size to bounding boxes

* fix annotation collation

* appease mypy

* add benchmark

* always use image as reference

* another refactor

* add support for segmentations

* add support for segmentations

* fix CI dependencies
@jdsgomes
Feature extraction default arguments - ops (#4810)
a7a3675 
making torchvision ops leaf nodes by default
@pmeier @prabhat00155
[FBcode->GH] remove unused requests functionality (#5014)
33123be 
Co-authored-by: Prabhat Roy <prabhatroy@fb.com>
@kazhang
cover VideoReader
51f015a
@kazhang
cover c++ APIs
dedf489
@NicolasHug @datumbox
Add bias parameter to ConvNormActivation (#5012)
a370e79 
* Add bias parameter to ConvNormActivation

* Update torchvision/ops/misc.py

Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>

Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
@ppwwyyxx @NicolasHug @fmassa
Change batched NMS threshold to choose for-loop version (#4990)
0aa3717 
According to the benchmark link #1311 (comment), for GPU the threshold should be higher (which is why detectron2 used 40k).
This PR changes the threshold to be device dependent.

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
@pmeier
remove vanilla tensors from prototype datasets samples (#5018)
874581c
@datumbox
Fix linebreak dos-newlines issue. (#5020)
23ca7e7
@pmeier
add pre-commit hook to fix line endings (#5021)
fe4ba30
@kbozas
support amp training for video classification models (#5023)
ba299e8 
* support amp training for video classification models

* Removed extra empty line and used scaler instead of args.amp as function argument

* apply formating to pass lint tests

Co-authored-by: Konstantinos Bozas <kbz@kbz-mbp.broadband>
@pmeier
fix fromfile on windows (#4980)
dcf5dc8 
* exclude windows from mmap

* add comment for windows

* appease mypy
@prabhat00155
Skip test_roi_align_aligned (#5029)
a3d8719
@prabhat00155
Add missing space in error message (#5031)
9b57de6
@NicolasHug
Add RAFT model for optical flow (#5022)
01ffb3a
datumbox
datumbox approved these changes Dec 6, 2021
View reviewed changes
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.

I have a few questions up for discussion, other than that the changes look good to me.

torchvision/csrc/io/image/cpu/decode_png.cpp Outdated
const torch::Tensor& data,
ImageReadMode mode,
bool allow_16_bits) {
C10_LOG_API_USAGE_ONCE("torchvision.io.decode_png");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here and in all other places where the string on the C++ side matches the one on Python.

prabhat00155
prabhat00155 approved these changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@prabhat00155 prabhat00155 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!

Borda and others added 4 commits December 6, 2021 17:01
@Borda @NicolasHug
adding PR template (#4586)
93b26da 
* adding PR template

* update

* Update PULL_REQUEST_TEMPLATE.md

* Update PULL_REQUEST_TEMPLATE.md

* Update .github/PULL_REQUEST_TEMPLATE.md

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
@kazhang
add api usage log for io
c610fbd
@kazhang
cover VideoReader
d227aff
@kazhang
cover c++ APIs
ed7513d
datumbox
datumbox suggested changes Dec 7, 2021
View reviewed changes
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.

@kazhang It seems that something is wrong with the PR: 78 modified files and 3.1k additions/1.6k deletions.

I haven't checked which commit caused this but perhaps its worth force pushing to undo or starting a new one. Marking as "request changes" to avoid accidental merges while the PR is in this state.

@kazhang kazhang closed this Dec 7, 2021
@kazhang
Copy link
Contributor Author

kazhang commented Dec 7, 2021

@datumbox I created a new PR: #5038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fmassa fmassa Awaiting requested review from fmassa

2 more reviewers

@datumbox datumbox datumbox requested changes

@prabhat00155 prabhat00155 prabhat00155 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ciflow/default cla signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add _log_api_usage_once() calls on remaining endpoints

10 participants

@kazhang @facebook-github-bot @datumbox @prabhat00155 @pmeier @jdsgomes @NicolasHug @ppwwyyxx @kbozas @Borda