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

[Refactor] Use pycocotools instead of mmpycocotools #4939

Merged
merged 5 commits into from
Apr 14, 2021

Conversation

xvjiarui
Copy link
Collaborator

@xvjiarui xvjiarui commented Apr 12, 2021

Motivation:

mmpycocotools and official pycocotool have the same package name. Users install pycocotools first fail to run MMDetection.
As the pycocotool is maintained by the official team now, we decide to deprecate the mmpycocotool. To make official pycocotools works with MMDetection, we add some function aliases for pycocotools.coco.COCO.

Besides, this PR makes users able to install MMDetection and Detectron2 under the same environment.

Fixed #4565

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #4939 (6467d7d) into master (5ebba9a) will decrease coverage by 0.17%.
The diff coverage is 53.77%.

❗ Current head 6467d7d differs from pull request most recent head bc0b69a. Consider uploading reports for the commit bc0b69a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4939      +/-   ##
==========================================
- Coverage   65.63%   65.46%   -0.18%     
==========================================
  Files         255      257       +2     
  Lines       19984    20053      +69     
  Branches     3394     3406      +12     
==========================================
+ Hits        13117    13127      +10     
- Misses       6165     6219      +54     
- Partials      702      707       +5     
Flag Coverage Δ
unittests 65.45% <53.77%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmdet/core/bbox/assigners/region_assigner.py 12.87% <0.00%> (-0.82%) ⬇️
mmdet/models/dense_heads/vfnet_head.py 31.48% <0.00%> (ø)
mmdet/datasets/lvis.py 15.17% <12.50%> (+1.17%) ⬆️
...mdet/core/bbox/iou_calculators/iou2d_calculator.py 82.50% <40.90%> (-14.28%) ⬇️
mmdet/datasets/custom.py 63.47% <65.38%> (+0.35%) ⬆️
mmdet/datasets/api_wrappers/coco_api.py 87.50% <87.50%> (ø)
mmdet/datasets/api_wrappers/__init__.py 100.00% <100.00%> (ø)
mmdet/datasets/coco.py 46.25% <100.00%> (-0.06%) ⬇️
mmdet/models/dense_heads/fcos_head.py 56.69% <100.00%> (+0.39%) ⬆️
mmdet/models/roi_heads/mask_scoring_roi_head.py 55.35% <0.00%> (-32.15%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ebba9a...bc0b69a. Read the comment docs.

@ZwwWayne
Copy link
Collaborator

ZwwWayne commented Apr 14, 2021

The refactoring LGTM. We should also tell users to switch to pycocotools if they already install mmpycocotools.

@xvjiarui
Copy link
Collaborator Author

PR message updated.

@xvjiarui xvjiarui requested a review from ZwwWayne April 14, 2021 03:50
## pycocotools compatibility

Before [PR 4939](https://github.com/open-mmlab/mmdetection/pull/4939), since `pycocotools` and `mmpycocotool` have the same package name, if users already installed `pyccocotools` (installed Detectron2 first under the same environment), then the setup of MMDetection will skip installing `mmpycocotool`. Thux MMDetection fails due to the missing `mmpycocotools`.
[PR 4939](https://github.com/open-mmlab/mmdetection/pull/4939) deprecates mmpycocotools in favor official pycocotools.
Copy link
Collaborator

Choose a reason for hiding this comment

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

in favor of


## pycocotools compatibility

Before [PR 4939](https://github.com/open-mmlab/mmdetection/pull/4939), since `pycocotools` and `mmpycocotool` have the same package name, if users already installed `pyccocotools` (installed Detectron2 first under the same environment), then the setup of MMDetection will skip installing `mmpycocotool`. Thux MMDetection fails due to the missing `mmpycocotools`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indicate that mmpycocotools works for both MMDet and Detectron2, so it will be fine if MMDetection is installed before Detectron2.

docs/compatibility.md Outdated Show resolved Hide resolved
@ZwwWayne ZwwWayne merged commit 5795367 into open-mmlab:master Apr 14, 2021
@@ -277,17 +278,15 @@ def load_annotations(self, ann_file):

try:
import lvis
assert lvis.__version__ >= '10.5.3'
if lvis.__version__ >= '10.5.3':
Copy link
Contributor

Choose a reason for hiding this comment

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

This will trigger an AttributeError for official LVIS

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thx! Fixed in #4958

Comment on lines +18 to +21
if not getattr(pycocotools, '__version__', '0') >= '12.0.2':
warnings.warn(
'mmpycocotools is deprecated. Please install official pycocotools by "pip install pycocotools"', # noqa: E501
UserWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will print a warning for official pycocotools

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thx! Fixed in #4958

@fcakyon
Copy link
Contributor

fcakyon commented Apr 15, 2021

@xvjiarui @ppwwyyxx @ZwwWayne The official pycocotools does not provide windows support because of this line in setup.py, how to use mmdetection on windows after this pr?

@lucasjinreal
Copy link

@fcakyon

ERROR: Could not find a version that satisfies the requirement pycocotools-windows (from mmdet) (from versions: none)
ERROR: No matching distribution found for pycocotools-windows```

@fcakyon
Copy link
Contributor

fcakyon commented Dec 20, 2021

@jinfagang i have opened a pr explaining the situation: #6838

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve conflicts between pycocotools and mmpycocotools
5 participants