-
Notifications
You must be signed in to change notification settings - Fork 28
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
Specify map location when loading model #272
Changes from all commits
01dd85b
ae380bd
4da92da
6c211f1
34af345
64fc0dd
4f912c8
9fa7e86
c3e08e2
754afa2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -303,5 +303,6 @@ def to_disk(self, path: os.PathLike): | |
torch.save(checkpoint, path) | ||
|
||
@classmethod | ||
def from_disk(cls, path: os.PathLike): | ||
return cls.load_from_checkpoint(path) | ||
def from_disk(cls, path: os.PathLike, **kwargs): | ||
# note: we always load models onto CPU; moving to GPU is handled by `devices` in pl.Trainer | ||
return cls.load_from_checkpoint(path, map_location="cpu", **kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this is fine, but forcing cpu here still feels a little weird to me. What if someone isn't using the trainer? Then they have to know and move devices themselves? Just wondering if letting things happen automatically is it a problem? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With the recent torch changes, we need to specify a map location when loading from checkpoint (map_location cannot be None; that is the source of the failing tests). Trying to figure out if the user has a GPU and wants to use it at this point in the code just adds redundancy and complexity in a way that is not helpful (it's hard to summarize succinctly here but that is the route we initially tried and it was not a good option).
Whether they're using our |
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.
[cleanup] removed unused params