8000
Skip to content

Improves more optional module imports#367

Merged
zhiqwang merged 3 commits intozhiqwang:mainfrom
mattpopovich:improve-more-imports
Mar 18, 2022
Merged

Improves more optional module imports#367
zhiqwang merged 3 commits intozhiqwang:mainfrom
mattpopovich:improve-more-imports

Conversation

@mattpopovich
Copy link
Copy Markdown
Contributor
@mattpopovich mattpopovich commented Mar 15, 2022

Builds on #366 to remove more ImportErrors. Not sure if you wanted these done as well so I made it into an additional PR.

There are a few ImportErrors remaining, it wasn't clear to me if I should remove those as well or keep them. I'll leave that to you to decide.

@CLAassistant
Copy link
Copy Markdown
CLAassistant commented Mar 15, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

zhiqwang
zhiqwang reviewed Mar 16, 2022
View reviewed changes
Copy link
Copy Markdown
Owner
@zhiqwang zhiqwang left a comment

Choose a reason for hiding this comment

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

Hi @mattpopovich , this also looks GREAT to me, but let's merge #366 first!

@mattpopovich
Copy link
Copy Markdown
Contributor Author
mattpopovich commented Mar 16, 2022

Agreed. Let me know if you have any thoughts/suggestions on how to fix that circular reference... I didn't look into it too closely yet.

@zhiqwang zhiqwang added enhancement New feature or request code quality Code format and unit tests dependencies Pull requests that update a dependency file labels Mar 16, 2022
@zhiqwang
Copy link
Copy Markdown
Owner
zhiqwang commented Mar 16, 2022 •
edited
Loading

Let me know if you have any thoughts/suggestions on how to fix that circular reference... I didn't look into it too closely yet.

Hi @mattpopovich , that's my bad, I mistakenly put the following two functions in the current position, and these two dependencies will cause the circular reference calls. I think it should be better to move load_from_ultralytics() into "yolort/models/_utils.py" and move contains_any_tensor into "yolort/utils/__init__.py" to solve this problem entirely.

https://github.com/zhiqwang/yolov5-rt-stack/blob/9a27b916da21a0143eb5497ee7ec6dced69f2358/yolort/utils/update_module_state.py#L40

https://github.com/zhiqwang/yolov5-rt-stack/blob/9a27b916da21a0143eb5497ee7ec6dced69f2358/yolort/data/_helper.py#L52

I'll submit a PR to resolve this issue.

@zhiqwang zhiqwang mentioned this pull request Mar 16, 2022
Closed
@zhiqwang zhiqwang force-pushed the improve-more-imports branch from 006654e to 9f1e376 Compare March 16, 2022 09:37
@zhiqwang
Copy link
Copy Markdown
Owner
zhiqwang commented Mar 16, 2022 •
edited
Loading

Hi @mattpopovich ,

After #369 is merged, I guess the circular import error will be resolved, I rebase manually this branch to the main, let's test these on the CI again.

Because of my manual rebase, the code now has some conflicts with your branch, please use the following command to synchronize with the current branch:

git pull --rebase origin improve-more-imports

@codecov
Copy link
Copy Markdown
codecov bot commented Mar 16, 2022 •
edited
Loading

Codecov Report

Merging #367 (37a1fc2) into main (bf05b4b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #367   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files          11       11           
  Lines         778      778           
=======================================
  Hits          767      767           
  Misses         11       11
Flag Coverage Ξ”
unittests 98.58% <ΓΈ> (ΓΈ)

Flags with carried forward coverage won't be shown. Click here to find out 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 bf05b4b...37a1fc2. Read the comment docs.

@zhiqwang zhiqwang force-pushed the improve-more-imports branch from ff6bc96 to a295096 Compare March 17, 2022 13:02
zhiqwang
zhiqwang reviewed Mar 17, 2022
View reviewed changes
Copy link
Copy Markdown
Owner
@zhiqwang zhiqwang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @mattpopovich , the overall looks great to me, I only add some comments about where to place the decorator, let me know if you have more concerns here.

And after #366 is merged, I only cherry-pick one commit from your commits history (the others have been merged to the main branch), and now you should use following command to synchronize with the current branch:

git pull --rebase origin improve-more-imports

@mattpopovich
Copy link
Copy Markdown
Contributor Author
mattpopovich commented Mar 18, 2022

@zhiqwang your requested changes have been made, CI looks good πŸ‘

zhiqwang
zhiqwang approved these changes Mar 18, 2022
View reviewed changes
Copy link
Copy Markdown
Owner
@zhiqwang zhiqwang left a comment

Choose a reason for hiding this comment

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

Thanks @mattpopovich again, this looks great to me!

@zhiqwang zhiqwang merged commit 6576f6c into zhiqwang:main Mar 18, 2022
@mattpopovich mattpopovich deleted the improve-more-imports branch March 18, 2022 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhiqwang zhiqwang zhiqwang approved these changes

Assignees

No one assigned

Labels

code quality Code format and unit tests dependencies Pull requests that update a dependency file enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mattpopovich @CLAassistant @zhiqwang
0