CMakeLists and C++ files refactoring. #2446

vcarpani · 2020-07-09T18:06:43Z

This PR refactors the CMakeLists file and the C++ files, to make it easier to package the C++ version of this library.

The main problem is due to the usage of INTERFACE_INCLUDE_DIRECTORIES in the CMakeLists file, since it is not advisable to populate the INSTALL_INTERFACE of the INTERFACE_INCLUDE_DIRECTORIES of a target with paths for dependencies. That would hard-code into installed packages the include directory paths for dependencies as found on the machine the package was made on [source].

Changes:

Moved cpp headers to cinclude folder.
Refactor cpp source files to account for the move of headers file.
Use cmake include_directories instead of INTERFACE_INCLUDE_DIRECTORIES

There is still some work to be done on the ROCM version of the python library.

codecov · 2020-07-09T19:50:13Z

Codecov Report

Merging #2446 into master will decrease coverage by 1.88%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2446      +/-   ##
==========================================
- Coverage   70.65%   68.77%   -1.89%     
==========================================
  Files          94       94              
  Lines        7897     7897              
  Branches     1241     1241              
==========================================
- Hits         5580     5431     -149     
- Misses       1934     2076     +142     
- Partials      383      390       +7

Impacted Files	Coverage Δ
torchvision/ops/_register_onnx_ops.py	`50.00% <0.00%> (-28.95%)`	⬇️
torchvision/io/_video_opt.py	`21.25% <0.00%> (-26.25%)`	⬇️
torchvision/models/detection/transform.py	`78.33% <0.00%> (-17.23%)`	⬇️
torchvision/ops/poolers.py	`85.14% <0.00%> (-11.89%)`	⬇️
torchvision/ops/boxes.py	`73.33% <0.00%> (-11.12%)`	⬇️
torchvision/datasets/video_utils.py	`68.48% <0.00%> (-7.28%)`	⬇️
torchvision/__init__.py	`71.87% <0.00%> (-6.25%)`	⬇️
torchvision/models/detection/roi_heads.py	`77.23% <0.00%> (-5.11%)`	⬇️
torchvision/models/detection/rpn.py	`89.69% <0.00%> (-3.44%)`	⬇️
torchvision/io/video.py	`68.63% <0.00%> (-1.19%)`	⬇️

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 86b6c3e...6f45098. Read the comment docs.

fmassa

Hi,

Thanks for the PR!

Just so that I understand a bit better, in this PR do you target the models or the operators most specifically?
I don't have much experience with CMake, so I won't be able to review this PR. Also, how does it compare to #2253 ?

Maybe @bmanga could have a look?

fmassa · 2020-07-10T08:35:58Z

CMakeLists.txt

@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.0)
 project(torchvision)
 set(CMAKE_CXX_STANDARD 14)
-set(TORCHVISION_VERSION 0.6.0)
+set(TORCHVISION_VERSION 0.5.0)


this seems like an unwanted change?

Yes, sorry for the typo

vcarpani · 2020-07-10T09:03:09Z

Hi,

Thanks for the PR!

Just so that I understand a bit better, in this PR do you target the models or the operators most specifically?
I don't have much experience with CMake, so I won't be able to review this PR. Also, how does it compare to #2253 ?

Maybe @bmanga could have a look?

Hi, thanks for the quick feedback!

This PR targets all the C++ code, a part from decoder, image and video_reader. It is not directly related with #2253, but if #2253 gets merged before this, this PR is going to need some more work or viceversa.

vcarpani · 2020-07-28T12:08:56Z

Any news on this?

bmanga · 2020-07-30T10:47:15Z

Hi @vcarpani, can you edit the PR message to list all the changes you made? Thanks!

bmanga · 2020-07-30T10:56:30Z

CMakeLists.txt


+# Gather source files.


I'm not a fan of these kinds of comments, they don't add anything valuable.

bmanga · 2020-07-30T11:03:34Z

CMakeLists.txt

+    "${CMAKE_CURRENT_BINARY_DIR}/TorchVisionConfig.cmake"
+    INSTALL_DESTINATION ${TORCHVISION_CMAKECONFIG_INSTALL_DIR}
+)

 write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/TorchVisionConfigVersion.cmake
-  VERSION ${TORCHVISION_VERSION}
-  COMPATIBILITY AnyNewerVersion)
+    VERSION ${TORCHVISION_VERSION}
+    COMPATIBILITY AnyNewerVersion
+)

 install(FILES ${CMAKE_CURRENT_BINARY_DIR}/TorchVisionConfig.cmake
-  ${CMAKE_CURRENT_BINARY_DIR}/TorchVisionConfigVersion.cmake
-  DESTINATION ${TORCHVISION_CMAKECONFIG_INSTALL_DIR})
+    ${CMAKE_CURRENT_BINARY_DIR}/TorchVisionConfigVersion.cmake
+    DESTINATION ${TORCHVISION_CMAKECONFIG_INSTALL_DIR}
+)

+# Install library.
 install(TARGETS ${PROJECT_NAME}
-  EXPORT TorchVisionTargets
-  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
-  )
+    EXPORT TorchVisionTargets
+    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+)

 install(EXPORT TorchVisionTargets
-  NAMESPACE TorchVision::
-  DESTINATION ${TORCHVISION_CMAKECONFIG_INSTALL_DIR})
+    NAMESPACE TorchVision::
+    DESTINATION ${TORCHVISION_CMAKECONFIG_INSTALL_DIR}
+)


These all look like unrelated formatting changes. I'd say stick to the current style, and if you want to propose a different one do so in a different PR.

vcarpani · 2020-07-31T13:24:27Z

Hi @bmanga, last commit should solve your comments, plus I added the list of changes to the description of the PR

fmassa · 2020-08-03T13:13:10Z

Naive question: would it be possible to avoid moving the header files into a new directory and concentrate the changes only into CMakeLists?

vcarpani · 2020-08-03T13:17:05Z

Naive question: would it be possible to avoid moving the header files into a new directory and concentrate the changes only into CMakeLists?

From my knowledge of CMake not, but I could look into that somewhere this week. From my point of view about C++, it is always nice to have a clear split between headers and sources and CMake makes it really easy to do that

fmassa · 2020-08-03T15:03:46Z

The reason why I'm asking is because this change will require some internal changes as well to torchvision, and in PyTorch they do mix headers and implementation in the same folder, so I would say I would prefer if we could keep it that way, specially given that in the current state of the code the implementation lives in header files.

bmanga · 2020-08-04T11:09:17Z

@vcarpani have you tried to include_directories(torchvision/csrc)?

vcarpani · 2020-08-04T11:15:12Z

@bmanga that would probably work (it is also the same approach the pytorch uses), but it is also quite horrible, since you would also get the cpp files in the include directories

bmanga · 2020-08-04T11:26:52Z

@vcarpani if you mean during installation, you can specify a PATTERN for cmake's install command.

vcarpani · 2020-08-04T11:30:42Z

@bmanga, yes I was referring to that, since it forces you to specify the installation pattern, while it is much simpler if you have only .h or .hpp files in the include_directories

bmanga · 2020-08-05T13:01:38Z

@vcarpani I understand your point and I agree in principle, however I don't think it's a worthwhile change as of now. I'd suggest to go for the include_directories(torchvision/csrc) + install pattern for this PR. It should address the problem with INTERFACE_INCLUDE_DIRECTORIES and be minimally disruptive.

fmassa · 2020-08-05T16:17:47Z

I agree with @bmanga comment, @vcarpani could you change your PR to do what @bmanga proposed?

vcarpani and others added 5 commits July 9, 2020 19:58

Split src and include dirs.

fa58d9c

Adapt python setup to new cpp structure.

8e0b151

Do not use previously found sources as glob pattern.

ce80bfb

Fix setup.py for image reading extension.

2de8972

Fixes for pylint.

80bf45b

fmassa reviewed Jul 10, 2020

View reviewed changes

Set proper torchvision version

06e1867

bmanga reviewed Jul 30, 2020

View reviewed changes

vcarpani force-pushed the fizyr-master branch from d570905 to 8f0065b Compare July 31, 2020 13:17

Remove formatting changes.

6f45098

vcarpani force-pushed the fizyr-master branch from 8f0065b to 6f45098 Compare July 31, 2020 13:23

vcarpani mentioned this pull request Aug 14, 2020

Use include_directories, to help packaging of C++. #2587

Merged

vcarpani closed this Aug 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMakeLists and C++ files refactoring. #2446

CMakeLists and C++ files refactoring. #2446

vcarpani commented Jul 9, 2020 •

edited

Loading

codecov bot commented Jul 9, 2020 •

edited

Loading

fmassa left a comment

fmassa Jul 10, 2020

vcarpani Jul 10, 2020

vcarpani commented Jul 10, 2020

vcarpani commented Jul 28, 2020

bmanga commented Jul 30, 2020

bmanga Jul 30, 2020

bmanga Jul 30, 2020

vcarpani commented Jul 31, 2020

fmassa commented Aug 3, 2020 •

edited

Loading

vcarpani commented Aug 3, 2020 •

edited

Loading

fmassa commented Aug 3, 2020 •

edited

Loading

bmanga commented Aug 4, 2020

vcarpani commented Aug 4, 2020

bmanga commented Aug 4, 2020

vcarpani commented Aug 4, 2020

bmanga commented Aug 5, 2020

fmassa commented Aug 5, 2020

CMakeLists and C++ files refactoring. #2446

CMakeLists and C++ files refactoring. #2446

Conversation

vcarpani commented Jul 9, 2020 • edited Loading

codecov bot commented Jul 9, 2020 • edited Loading

Codecov Report

fmassa left a comment

Choose a reason for hiding this comment

fmassa Jul 10, 2020

Choose a reason for hiding this comment

vcarpani Jul 10, 2020

Choose a reason for hiding this comment

vcarpani commented Jul 10, 2020

vcarpani commented Jul 28, 2020

bmanga commented Jul 30, 2020

bmanga Jul 30, 2020

Choose a reason for hiding this comment

bmanga Jul 30, 2020

Choose a reason for hiding this comment

vcarpani commented Jul 31, 2020

fmassa commented Aug 3, 2020 • edited Loading

vcarpani commented Aug 3, 2020 • edited Loading

fmassa commented Aug 3, 2020 • edited Loading

bmanga commented Aug 4, 2020

vcarpani commented Aug 4, 2020

bmanga commented Aug 4, 2020

vcarpani commented Aug 4, 2020

bmanga commented Aug 5, 2020

fmassa commented Aug 5, 2020

vcarpani commented Jul 9, 2020 •

edited

Loading

codecov bot commented Jul 9, 2020 •

edited

Loading

fmassa commented Aug 3, 2020 •

edited

Loading

vcarpani commented Aug 3, 2020 •

edited

Loading

fmassa commented Aug 3, 2020 •

edited

Loading