Skip to content
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

Object detection pipeline #12886

Merged
merged 41 commits into from
Sep 8, 2021

Conversation

mishig25
Copy link
Contributor

@mishig25 mishig25 commented Jul 26, 2021

What does this PR do?

  • Object detection pipeline
  • Give an image or list of images, outputs obj det annotations in form:
[
    [
        {'score': 0.9..., 'label': 'remote', 'box': [{'x': 66, 'y': 118}, ...},
    ],
    ...
]
  • See colab for more details

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@NielsRogge

@mishig25 mishig25 requested a review from NielsRogge July 26, 2021 08:44
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
mishig25 and others added 8 commits July 26, 2021 19:53
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
@mishig25 mishig25 added the Core: Pipeline Internals of the library; Pipeline. label Jul 27, 2021
@mishig25 mishig25 marked this pull request as ready for review July 27, 2021 12:34
Copy link
Contributor

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

mishig25 and others added 3 commits September 6, 2021 12:41
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
mishig25 and others added 2 commits September 6, 2021 12:46
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
mishig25 and others added 3 commits September 6, 2021 12:50
Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>
@mishig25
Copy link
Contributor Author

mishig25 commented Sep 6, 2021

@LysandreJik I think its ready to be merged. Please let me know if you there's anything else I need to take care of :)

@Narsil
Copy link
Contributor

Narsil commented Sep 7, 2021

Hi @mishig25 ,

I think you need to fix all the tests.

import torch need to be protected behind is_torch_available for instance.
For code quality you can pip install -e .[dev] and then make fixup.
The PT tests also seem to require timm which are not available in the tests. So you need a @require_timm decorator.

@mishig25
Copy link
Contributor Author

mishig25 commented Sep 7, 2021

@Narsil I'm confused about the tf tests failing.
For example, in this failed test, I see the test failing for pipelines I haven't made any changes (also, I made sure my branch is up-to-date with the master):
here is an example for test_pipelines_translation

_____________ ERROR collecting tests/test_pipelines_translation.py _____________ImportError while importing test module '/home/circleci/transformers/tests/test_pipelines_translation.py'....E ModuleNotFoundError: No module named 'torch'
Please let me know what step I'm missing

@mishig25 mishig25 merged commit 2a15e8c into huggingface:master Sep 8, 2021
@mishig25
Copy link
Contributor Author

mishig25 commented Sep 8, 2021

Since the PR was approved by two HF members and tests passed, I've merged it when the merge option became available. Please let me know if it is a correct procedure (i.e. should I have waited until a transfomers maintainer merged it?)

@sgugger
Copy link
Collaborator

sgugger commented Sep 8, 2021

That's correct: as long as you have approval of one core maintainer (more for big PRs), addressed all comments, and all tests pass, you can merge your PR. :-)

@mishig25 mishig25 mentioned this pull request Sep 10, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core: Pipeline Internals of the library; Pipeline.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants