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] Fix potential circular import in core, data and utils, disconnect unnecessary pytorch3d dependency #190

Merged
merged 9 commits into from
Jun 20, 2022

Conversation

LazyBusyYang
Copy link
Collaborator

@LazyBusyYang LazyBusyYang commented May 30, 2022

  • Move mmhuman3d.core.visualization.renderer to mmhuman3d.core.renderer
  • Split mmhuman3d.core.renderer.torch3d_renderer.builder into several seperated builders.
  • Make __init__.py in mmhuman3d.core.renderer, mmhuman3d.data, mmhuman3d.utils empty.
  • Allow pytorch3d import failure in mmhuman3d.utils.transforms, because the transforms.py is widely imported by many modules that do not need pytorch3d at all (transforms.py is introduced by demo_utils.py).
  • Fix failed tests caused by the changes above.
  • Accept mmcv 1.5.2.

Below are some cases indicates what is different in this PR.

case 1. When we are trying to import visualize_kp2d
before this PR:
All the torch3d renderers, lights and shaders, vedo_renderer are imported because of __init__.py.
If pytorch3d has not been installed(e.g. OS or hardware not supported), program exits because of the module we never use.
after this PR:
Only the modules related to visualize_kp2d are imported.
If pytorch3d has not been installed, the program casts a warning message of import error and call stack, goes on, allows you to visualize kp2d.

case 2. When we are trying to import ParametricMeshes
before this PR:
All the torch3d renderers, lights and shaders, matplotlib_renderer, vedo_renderer are imported because of __init__.py.
after this PR:
Only the ParametricMeshes is imported, with a price of longer import sentence.

case 3. When we are trying to import build_renderer
before this PR:
from mmhuman3d.core.visualization.renderer import build_renderer.
after this PR:
from mmhuman3d.core.renderer.torch3d_renderer.builder import build_renderer. A little bit longer.

case 4. When we are trying to import MeshRenderer, PointCloudRenderer
before this PR:
from mmhuman3d.core.visualization.renderer import MeshRenderer, PointCloudRenderer.
after this PR:
from mmhuman3d.core.renderer.torch3d_renderer.builder import MeshRenderer, PointCloudRenderer. Classes in same registry can be accessed in builder, one line import.

case 5. When we are trying to import HumanData
before this PR:
If pytorch3d has not been installed, program exits because that it is imported by SMCReader.
after this PR:
If pytorch3d has not been installed, nothing happens. You can use the module as you wish.

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #190 (e1a8699) into main (4164f6a) will decrease coverage by 0.19%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   85.24%   85.05%   -0.20%     
==========================================
  Files         170      173       +3     
  Lines       14832    14826       -6     
==========================================
- Hits        12644    12610      -34     
- Misses       2188     2216      +28     
Flag Coverage Δ
unittests 85.05% <87.50%> (-0.20%) ⬇️

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

Impacted Files Coverage Δ
...3d/core/renderer/torch3d_renderer/lights/lights.py 88.13% <ø> (ø)
.../core/renderer/torch3d_renderer/normal_renderer.py 91.30% <ø> (ø)
...e/renderer/torch3d_renderer/pointcloud_renderer.py 78.26% <ø> (ø)
...d/core/renderer/torch3d_renderer/shader/builder.py 100.00% <ø> (ø)
...3d/core/renderer/torch3d_renderer/shader/shader.py 88.46% <ø> (ø)
...e/renderer/torch3d_renderer/silhouette_renderer.py 92.00% <ø> (ø)
...ore/renderer/torch3d_renderer/textures/textures.py 35.71% <ø> (ø)
mmhuman3d/core/renderer/torch3d_renderer/utils.py 70.96% <ø> (ø)
...an3d/core/renderer/torch3d_renderer/uv_renderer.py 85.59% <ø> (ø)
mmhuman3d/core/renderer/vedo_render.py 86.36% <ø> (ø)
... and 26 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 4164f6a...e1a8699. Read the comment docs.

@LazyBusyYang
Copy link
Collaborator Author

Import time compare:

import content import time(main, sec) import time(PR#190, sec) speed-up (x)
convert_kps 1.9779 1.9305 1.0246
visualize_kp2d 5.0012 3.7096 1.3482
visualize_kp3d 4.8724 3.7236 1.3085
images_to_array 2.7962 2.0348 1.3742
check_path_suffix 2.8509 1.9142 1.4893
HumanData 5.0130 1.8973 2.6422
visualize_smpl_calibration 4.8914 4.0477 1.2084
MeshRenderer 5.0428 1.8929 2.6641
ParametricMeshes 5.0820 2.7389 1.8555
MMLights 4.8773 1.9466 2.5055
build_shader 4.7687 2.0013 2.3828
build_renderer 5.0584 2.8075 1.8017

@LazyBusyYang
Copy link
Collaborator Author

Import failure compare if pytorch3d is not installed

S: success, F: failure, W: success with a warning.

import content result (main, S/F/W) result (PR#190, S/F/W)
convert_kps S S
visualize_kp2d F W
visualize_kp3d F W
images_to_array F S
check_path_suffix F S
HumanData F S
visualize_smpl_calibration F F
MeshRenderer F F
ParametricMeshes F F
MMLights F F
build_shader F F
build_renderer F F

@LazyBusyYang LazyBusyYang requested a review from ttxskk May 30, 2022 11:46
@LazyBusyYang LazyBusyYang changed the title [Fix] Fix potential circular import in core, data and utils, utilize pytorch3d dependency [Fix] Fix potential circular import in core, data and utils, disconnect unnecessary pytorch3d dependency May 31, 2022
@caizhongang
Copy link
Collaborator

Is this file meant to be included in the main branch?

@LazyBusyYang
Copy link
Collaborator Author

Is this file meant to be included in the main branch?

d7079a3

No. Thanks for finding this mistake! Removed in d7079a3.

@@ -29,7 +29,7 @@
If you want to visualize a T-pose smpl or your poses do not have global_orient, you can do:
```python
import torch
from mmhuman3d.core.visualization import visualize_T_pose
from mmhuman3d.core.visualization. import visualize_T_pose
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here should be from mmhuman3d.core.visualization.visualize_smpl import visualize_T_pose ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 2dd48b7.

@ttxskk ttxskk self-requested a review June 9, 2022 07:32
Copy link
Collaborator

@ttxskk ttxskk left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, no more comments.

Copy link
Collaborator

@caizhongang caizhongang left a comment

Choose a reason for hiding this comment

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

Looks good.

@caizhongang caizhongang merged commit 21a8ce7 into open-mmlab:main Jun 20, 2022
@LazyBusyYang LazyBusyYang deleted the clean_vis_import branch June 20, 2022 06:13
ttxskk pushed a commit to ttxskk/mmhuman3d that referenced this pull request Jun 30, 2022
…ct unnecessary pytorch3d dependency (open-mmlab#190)

- Move mmhuman3d.core.visualization.renderer to mmhuman3d.core.renderer
- Split mmhuman3d.core.renderer.torch3d_renderer.builder into several seperated builders.
- Make __init__.py in mmhuman3d.core.renderer, mmhuman3d.data, mmhuman3d.utils empty.
- Allow pytorch3d import failure in mmhuman3d.utils.transforms, because the transforms.py is widely imported by many modules that do not need pytorch3d at all (transforms.py is introduced by demo_utils.py).
- Fix failed tests caused by the changes above.
- Accept mmcv 1.5.2.

Co-authored-by: gaoyang3 <gaoyang3@sensetime.com>
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.

3 participants