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

[Fix] Correct isort setup #288

Merged
merged 2 commits into from Jun 16, 2021
Merged

[Fix] Correct isort setup #288

merged 2 commits into from Jun 16, 2021

Conversation

AllentDan
Copy link
Member

@AllentDan AllentDan commented Jun 15, 2021

Hi, this is a PR for correcting isort setup.
Mainly changed:
modify mmdet to 'known_third_party' in setup.cfg and add tools to 'known_first_party' in setup.cfg

@innerlee
Copy link
Contributor

innerlee commented Jun 15, 2021

What's the reason cause for the previous "error"?

@AllentDan
Copy link
Member Author

@innerlee Not error, just code requirements....

Can be ignored acctually.

@AllentDan
Copy link
Member Author

@innerlee. I just want to add the following line:

from mmocr.datasets.pipelines.crop import crop_img # noqa: F401

to onnx2tensorrt.py. But found that mmdet should be the third party of this repo and tools should be added to the first party.

@innerlee
Copy link
Contributor

Okay, why tools folder matter? They are not packages

@AllentDan
Copy link
Member Author

AllentDan commented Jun 15, 2021

The tools folder is imported in some of my PRs... But I forgot to add it to 'known_first_party' of this repo... If not specified, isort will move lines to 'known_third_party'.

Actually, move 'mmdet' to 'known_third_party' may cause some conflicts to somebody working on new PRs. So, I am not sure if it should be done in this PR. Maybe it should be done by the owner of this repo.

@codecov
Copy link

codecov bot commented Jun 15, 2021

Codecov Report

Merging #288 (e4d7c83) into main (87a7dce) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #288   +/-   ##
=======================================
  Coverage   85.81%   85.81%           
=======================================
  Files         132      132           
  Lines        8824     8824           
  Branches     1233     1233           
=======================================
  Hits         7572     7572           
  Misses        954      954           
  Partials      298      298           
Flag Coverage Δ
unittests 85.81% <100.00%> (ø)

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

Impacted Files Coverage Δ
mmocr/apis/inference.py 77.92% <ø> (ø)
mmocr/apis/train.py 14.06% <ø> (ø)
mmocr/datasets/kie_dataset.py 88.23% <ø> (ø)
mmocr/datasets/ner_dataset.py 100.00% <ø> (ø)
mmocr/datasets/ocr_dataset.py 100.00% <ø> (ø)
mmocr/datasets/pipelines/custom_format_bundle.py 94.11% <ø> (ø)
mmocr/datasets/pipelines/dbnet_transforms.py 64.36% <ø> (ø)
mmocr/datasets/pipelines/kie_transforms.py 28.57% <ø> (ø)
mmocr/datasets/pipelines/loading.py 80.39% <ø> (ø)
mmocr/datasets/pipelines/ner_transforms.py 100.00% <ø> (ø)
... and 62 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 87a7dce...e4d7c83. Read the comment docs.

@innerlee
Copy link
Contributor

The tools folder is imported in some of my PRs

This part is what I was concerning about. Can this "import" be avoided in some way?

done by the owner of this repo.

We do not have such restrictions. Contributions of any form are always welcome.

@AllentDan
Copy link
Member Author

AllentDan commented Jun 15, 2021

Hi, @innerlee. It can be avoided through replacing "from tools.deployment.deploy_helper import sth" with "from .deploy_helper import sth" or "from deploy_helper import sth". What is your suggestion? Seems mmdet prefers the former.

@innerlee
Copy link
Contributor

tools are stand-alone scripts. If something are reusable, then they should live in the package. So my suggestion is to move deploy_helper inside mmocr/

@AllentDan
Copy link
Member Author

Hi, @innerlee. How about this. In this PR, I only do the following stuff:

  1. move mmdet to 'known_third_party'.
  2. add 'from mmocr.datasets.pipelines.crop import crop_img # noqa: F401' to onnx2tensorrt.py.

I will move deploy_helper.py to mmocr in the next PR which aims to evaluate exported onnx and tensorrt files. Related unit tests will be added then.

@innerlee
Copy link
Contributor

fair enough.

Its better to remove # noqa: F401 in later pr also.

@innerlee innerlee merged commit d57f279 into open-mmlab:main Jun 16, 2021
@AllentDan
Copy link
Member Author

Hi, "# noqa: F401" was used because crop_img was not used. However, if not importing this func, a registry error will occur.

@AllentDan
Copy link
Member Author

Just like here

@innerlee
Copy link
Contributor

Just like here

Its easy to add some meaningless/unharmful ops for them like assert crop_img is not None.
The previous examples should be cleaned also.

@AllentDan AllentDan deleted the isort branch June 18, 2021 07:20
gaotongxiao pushed a commit to gaotongxiao/mmocr that referenced this pull request Jul 15, 2022
* isort

* remove tools from setup.cfg
gaotongxiao pushed a commit to gaotongxiao/mmocr that referenced this pull request Jul 15, 2022
* isort

* remove tools from setup.cfg
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.

None yet

2 participants