-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Reduce GPU memory usage in CacheDataloader #1730
Conversation
If you have a lidar iphone, you can create a capture with polycam that has depth. |
By keeping on CPU , we should also expect some modest performance performance improvement due to reduced GPU transfer bandwidth usage, is that correct? |
Unfortunately, I don’t have a lidar iphone. I guess I can run a monocular depth prediction model |
This is also related to #1465 |
You could try this dataset for testing (had it lying around so thought it might help here). I only tested this with instantNGP though, so you will have to go through that dataparser. https://drive.google.com/file/d/1-8hXmkmtvHI36N6_WVV2uBZnTx39HFno/view?usp=sharing NOTE: The data is very nosy, so don't expect fantastic reconstructions. |
I have tested this and I worked with the SDF dataparser. I will, however, make it more general by also reducing memory usage by segmentation masks and etc. |
3a2bfbc
to
5c19d09
Compare
5c19d09
to
9f68fd2
Compare
The implementation is complete. It should be tested for all possible setups I guess. I just tested sdf-studio parser with neus-facto and vanilla nerfacto. |
I'd be curious to hear @machenmusik perspective on this PR since he has looked more into dataloader/device stuff. |
Stymied by Windows and torch 2 issues at the moment; once that is cleared up, I can hopefully try some other variations with this. |
(Looks like there are some conflicts now...) |
I finally got a chance to look at this with private dataset, doing the obvious conflict resolution, and merged into PR2003 where I assume performance impacts would be magnified. Caveats
With this PR: nerfacto-huge default
nerfacto default
Without this PR: nerfacto-huge default
nerfacto default
So if others can confirm output is same or better (which I think was done previously) I think this is good. Let me know if you'd like me to commit the conflict resolution here for clarity. (cc @tancik @jkulhanek) |
@machenmusik thanks! Do you, by any chance, also have GPU memory usage stats? |
No sorry, huge model saturates this machine anyway so wouldn't see anything there really |
Can we merge this? |
It would be great if someone can independently assess visual quality is not degraded, but I have no objections |
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.
LGTM
This PR implements functionality to allow Datasets to keep some tensors on CPU (same as "image") - reducing GPU memory requirements.
Currently, I moved the "depth_image" tensor to GPU.
Note, I haven't tested it yet. Which datasets should I test?