Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve Documentation - Fix code links and improve tutorial layout #6321
Improve Documentation - Fix code links and improve tutorial layout #6321
Changes from 28 commits
bf4509e
6fe1f9e
dfd76a7
efabadf
a4ea39e
3a9da1f
9fc5a68
4f88a16
91778f2
de4f46a
e968505
ee7b6ae
5ba3035
e1ed485
8295331
d65fb67
776bb99
9ae5d23
9f405b3
0724b5a
22de7e3
2f9d0c2
a522ed8
fc04d88
cd5cc98
0f2439d
80427df
9c2840e
43ededc
3f10bb3
3a0a6e9
a5b5aff
3d345e6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep this section? It refers to the
run()
function inmake_fragments.py
that calls all the previous functions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssheorey The reason I removed this is because the
run
method calls onlyprocess_single_fragment
, howeverprocess_single_fragment
is never discussed in documentation before. Also,run
method focuses on use of multiprocessing to speedup the code, but thats not really where open3d documentation focus is.I dived deeper into git history and originally
process_single_fragment
used to beprocess_fragments
which did all prep before calling previous functions for each fragment. It was documentated in Batch Processing section - https://github.com/isl-org/Open3D/pull/521/files.See http://www.open3d.org/docs/0.6.0/tutorial/ReconstructionSystem/make_fragments.html#batch-processing for easy check (although code lines went a little out of sync with docs here)
See http://www.open3d.org/docs/0.7.0/tutorial/ReconstructionSystem/make_fragments.html#batch-processing where this got changed to
run
method.Will it be better to revert my change but instead we show either
process_single_fragment
. ORprocess_single_fragment
andrun
method. This would go well with addition of few statements explaining use of multiprocessing to speedup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option 2 sounds good. Let's show both
process_single_fragment
andrun
and explain use of multiprocessing to speed up processing. Optionally the parallel execution inrun()
can be cleaned up in the example.joblib
can be replaced withconcurrent.futures.ProcessPoolExecutor
ormultiprocessing.pool
, which is part of the Python standard library.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssheorey Need your input. I tried replacing joblib, but in an earlier problem at work, I had faced similar issue where open3d hangs with multiprocessing. As per your comment here, #1552 (comment), I moved to forkserver and that is working fine. However, forkserver is not available on Windows, and in this usecase, spawn should be fine as well. Shall I go with this?
P.S. Was curious, and found joblib went with forkserver for same reason but later reverted back to
fork
-Later they implemented loky backend which circumvent this issue. https://joblib.readthedocs.io/en/latest/parallel.html#bad-interaction-of-multiprocessing-and-third-party-libraries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good idea. This will also be a reference to users about how to use Open3D with multiprocessing.
Just add a comment saying "Linux default method 'fork' cannot be used with multithreaded Open3D."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python_mp_spawn is using spawn as starting method.
python_mp_forkserver is using forkserver as starting method.
python_joblib_mp_backend is using joblib with multiprocessing backend.
python_joblib_mp_backend is using joblib with loky backend.
python_single_process is just main process.
Only for 4 fragments with each fragment made of 10 images, I get following measurements:-
Didnt make much of a difference, despite with 16 cores and 4 child processes. As this is an introductory example, just setting 1 would be simpler and easier for new users IMO.
Final committed code test
Multiprocess takes
Making fragments 0:05:11.274633
(15 cores)Singleprocess takes
Making fragments 0:32:18.475112
(OpenMP number of threads unchanged)