fix: remove emb net device handling, refactor get_numel #1186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
janfb
force-pushed
the
fix-device-warnings
branch
from
07f3fc0
to
06f83d5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1186 +/- ##
==========================================
- Coverage 84.53% 75.56% -8.97%
==========================================
Files 94 95 +1
Lines 7571 7576 +5
==========================================
- Hits 6400 5725 -675
- Misses 1171 1851 +680
Flags with carried forward coverage won't be shown. Click here to find out more.
|
janfb
force-pushed
the
fix-device-warnings
branch
from
06f83d5
to
3a82707
Compare
janfb
force-pushed
the
fix-device-warnings
branch
from
479c0bb
to
5afba70
Compare
michaeldeistler
approved these changes
Jul 2, 2024
|
Hi, I've been trying to use sbi and torch for not very long time. I used the following script to train SNPE. The input net and training data are all of device "cuda". Based on what you updated, does it mean that the SNPE should only trained on cpu? Here are the errors I got from it: I tried with cpu device as well, but got: I'm not sure if I understood it correctly, could you maybe help? I really appriciate that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Context
The starting point for this PR is #1161, the incorrect warning that embedding net and data device do not match.
On the way I realized that we are treating the
embedding_netas separate net that can have its own device, different from the actual net. I think this does not make sense.In general, the device handling should be centralized, e.g., have a single entry point. At the moment, this entry point is the inference object, e.g.,
SNPE(..., device=device). But are the different scenarios:SNPE: all good, device handling is centralized via thedevicedevicepassed toSNPE.posterior_nnto build a flow with an embedding net.posterior_nnnormally returns a net on the cpu. but if theembedding_netpassed by the user is on a different device, things might crash.My suggestions
EDIT: Does not make sense because in the standard case it will be in cpu and be moved to training device later, so there will be a mismatch. So we can either move the passed net to cpu entirely (bad), or, in those few cases where users pass large nets that have to be on the GPU, accept potential device mismatches.
posterior_nnetc, that the passed embedding net is on the cpu, or we move it there.EDIT: I will add a function that checks the embedding net device and if it is not on cpu, it warns and moves it there.
What this PR does so far
embedding_netdevice checkingbuild_posterior(and add test)get_numelto be used across the neural net factory. I had to put it into a separate utils file because putting it intosbiutilsortorchutilscauses circular imports 😵fixes #1161