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

Release Python GIL in Eigen API #5888

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Release Python GIL in Eigen API #5888

wants to merge 4 commits into from

Conversation

ssheorey
Copy link
Member

@ssheorey ssheorey commented Feb 3, 2023

Type

Motivation and Context

Release Python GIL in estimate normals, optimize pose graph, fpfh computation. Allow Python multithreading for these operations.

Update: Release Python GIL in most Eigen API functions for geometry and pipelines. Updates to visualization, ML and newer Tensor API modules is future work.

Note to users:

  • Do not rely on the Python GIL to protect user data (Open3D data structures). When using Python multi-threading, use separate locks to protect data from concurrent access. This PR may break user code that used Python GIL in this way earlier.
  • Many Open3D functions already use multithreading automatically based on the number of CPUs in the system. Programs will likely not see speed increase by calling multiple such Open3D functions in parallel. This PR does offer benefits of running IO (e.g. Open3D / Python file reading) and compute (e.g. Open3D registration) in parallel. Some Open3D compute intensive functions are single threaded and can benefit from being run in parallel as well. Check the CPU usage of your workloads to check if Python multithreading would be useful.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

This change is Reviewable

in estimate normals, optimize pose graph, fpfh computation. Allow Python multithreading for these operations.

Resolves #5837
@update-docs
Copy link

update-docs bot commented Feb 3, 2023

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@yuecideng
Copy link
Collaborator

Hi @ssheorey , should we also release the pyhton GIL for methods of tensor based PointCloud?

@ssheorey ssheorey added this to the v0.17 milestone Feb 5, 2023
@ssheorey ssheorey closed this Feb 8, 2023
@ssheorey ssheorey reopened this Feb 8, 2023
@ssheorey ssheorey changed the title Release Python GIL Release Python GIL in Eigen API Feb 8, 2023
@ssheorey
Copy link
Member Author

ssheorey commented Feb 8, 2023

Hi @ssheorey , should we also release the pyhton GIL for methods of tensor based PointCloud?

Updated PR to be more comprehensive. Added user notes about potential benefits and user warning.

@ssheorey ssheorey marked this pull request as draft February 8, 2023 17:00
@ssheorey ssheorey removed this from the v0.17 milestone Feb 17, 2023
@tiglieto
Copy link

tiglieto commented Jun 1, 2023

Pleasy please give it a watch?

I have a very involved program and calculating fpfh features on the main process would be very cumbersome.

Thanks to everyone for the wonderful work.

@theNded
Copy link
Contributor

theNded commented Aug 11, 2023

@ssheorey is there any significant changes required? Otherwise I think we can make it ready for review?

@ssheorey
Copy link
Member Author

@theNded This PR removed the GIL for all eigen based functions and many unit tests failed. I need to look at each of the failing unit tests and selectively remove GIL. It may take some time for me to get around to this.

@theNded
Copy link
Contributor

theNded commented Aug 11, 2023

@theNded This PR removed the GIL for all eigen based functions and many unit tests failed. I need to look at each of the failing unit tests and selectively remove GIL. It may take some time for me to get around to this.

I can do this if these are the only required changes.

@ssheorey
Copy link
Member Author

@theNded This PR removed the GIL for all eigen based functions and many unit tests failed. I need to look at each of the failing unit tests and selectively remove GIL. It may take some time for me to get around to this.

I can do this if these are the only required changes.

Yes that should be all. Thanks for picking this up!

@ssheorey ssheorey added this to the v0.20 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Please release python GIL in RegistrationGeneralizedICP
4 participants