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

Cleanup C++ model example and cmake CI job #8411

Merged
merged 19 commits into from
May 8, 2024

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented May 6, 2024

This PR cleans up some CMake stuff. I added comments to explain the changes made to the respective files.

This does not make any change to the CMakeLists.txt at the root of the repo, and doesn't change the way libtorchvision is built either. Those are only changes related to docs on how to build it, and related to the examples / tests we have.

The main changes are:

  • remove stuff from test/tracing/ and integrate them within example/cpp
  • remove unnecessary dependency on libpython for the examples
  • properly depend on libtorchvision in the example but only when needed - some models require it, some don't. Now we let the user choose whether they want it or not. This is better documented as well.

Copy link

pytorch-bot bot commented May 6, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8411

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 5041b90 with merge base d23a6e1 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@NicolasHug NicolasHug marked this pull request as ready for review May 7, 2024 15:59
Copy link
Member Author

Choose a reason for hiding this comment

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

This is executed by our Cmake CI job.

Originally this does the following:

  • build torchvisionlib
  • build and run the example/cpp stuff
  • build and run the test/tracing/frcnn stuff.

In this PR I removed the test/tracing/frcnn stuff and merged it into the example/cpp stuff, so in this file we're just removing the things related to test/tracing/frcnn.


cmake .. -DTorch_DIR="${Torch_DIR}" \
-DCMAKE_PREFIX_PATH="${CONDA_PREFIX}" \
-DCMAKE_FIND_FRAMEWORK=NEVER
-DCMAKE_FIND_FRAMEWORK=NEVER \
-DUSE_TORCHVISION=ON # Needed for faster-rcnn since it's using torchvision ops like NMS.
Copy link
Member Author

Choose a reason for hiding this comment

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

Some scripted models like Faster-RCNN rely on torchvision ops, so for those we need to link to libtorchvision. But some other models like resnet do not need libtorchvision.

Prior to this PR, the example's CmakeLists.txt would always link to libtorchvision even if it was unnecessary. I added a USE_TORCHVISION option to let use decide whether they want to link against it or not. Here, we want to test the RCNN model, so we have to pass USE_TORCHVISION=ON.

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is a merge of the previous hello_world/CMakeLists.txt (now deleted) and test/tracing/frcnn/CMakelists.txt (now deleted).

The main changes are:

  • only link to libtorchvision if USE_TORCHVISION is ON.
  • Do not link to libPython - this is actually not needed in this example regardless of the platform (it's only potentially required on libtorchvision on Windows).

Copy link
Member Author

Choose a reason for hiding this comment

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

Much more detailed and more accurate build instructions for libtorchvision and for the example.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a merge of the previous hello-world/main.cpp (deleted) and test/tracing/frcnn/test_frcnn_tracing.cpp (deleted).

This file is able to run both a resnet and faster-rcnn scripted model, and only include relevant headers if needed.

Comment on lines +6 to +8
#ifdef _WIN32
#include <torchvision/vision.h>
#endif // _WIN32
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't fully understand yet why this is required on Windows, but this is for another PR

Copy link
Member Author

Choose a reason for hiding this comment

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

like other files this is a merged of the previous stuff that was in hello-world and in tests/tracing.

@NicolasHug NicolasHug requested a review from pmeier May 7, 2024 16:15
@NicolasHug NicolasHug requested a review from vfdev-5 May 7, 2024 16:15
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Stamp

@NicolasHug NicolasHug merged commit 1644fff into pytorch:main May 8, 2024
89 checks passed
@NicolasHug NicolasHug deleted the cleanup_cpp_example branch May 8, 2024 09:05
Copy link

github-actions bot commented May 8, 2024

Hey @NicolasHug!

You merged this PR, but no labels were added.
The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request May 8, 2024
Reviewed By: vmoens

Differential Revision: D57099456

fbshipit-source-id: 8c8ed51549c4abaa27644bef5aae8484b6d3a057
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants