[C++ API] A few additions by ebetica · Pull Request #9837 · pytorch/pytorch

ebetica · 2018-07-25T19:27:42Z

This PR provides 4 fixes / features:

torch::nn::Cloneable inherits virtually from torch::nn::Module. We want to pass around a module with new functions, and the best way to do this is to do a diamond inheritance pattern, i.e.

struct MySuperModuleImpl : virtual public torch::nn::Module { 
  virtual void myFunction() = 0;
}

struct MySuperModule : public torch::nn::Cloneable<MySuperModule>, MySuperModuleImple {};

struct MyModule : public MySuperModule<MyModule> {
  void myFunction() override; 
};

This way, we can simply pass around MySuperModuleImpl around instead of torch::nn::Module.

Optimizer options are public now, since there's no way to decay the LR or modify it during training otherwise
Serialization functions creates autograd history and calls copy_! Bad!
Optimizers did not create buffers after add_parameters was called.

goldsborough · 2018-07-30T15:51:24Z

test failures look legit though, something in serialization

ebetica · 2018-07-30T17:36:26Z

Hmm, the only thing I changed was that grad() returns a Variable, I'll try changing that back.

goldsborough

LGTM, let's just get the tests working

test/cpp/api/sequential.cpp

goldsborough · 2018-08-07T18:47:35Z

@pytorchbot retest this please

facebook-github-bot

goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

torch/csrc/api/include/torch/serialization.h

        cputensor.data_ptr(),
        cputensor.numel() * cputensor.type().elementSizeInBytes());
-    tensor.data().copy_(cputensor.data());
+    tensor.copy_(cputensor);


ebetica · 2018-08-07T19:17:24Z

Could you change the tensor.copy_ in load to use the data instead of the variable? @goldsborough

facebook-github-bot

goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: This PR provides 4 fixes / features: 1. torch::nn::Cloneable inherits virtually from torch::nn::Module. We want to pass around a module with new functions, and the best way to do this is to do a diamond inheritance pattern, i.e. ```c++ struct MySuperModuleImpl : virtual public torch::nn::Module { virtual void myFunction() = 0; } struct MySuperModule : public torch::nn::Cloneable<MySuperModule>, MySuperModuleImple {}; struct MyModule : public MySuperModule<MyModule> { void myFunction() override; }; ``` This way, we can simply pass around MySuperModuleImpl around instead of torch::nn::Module. 2. Optimizer options are public now, since there's no way to decay the LR or modify it during training otherwise 3. Serialization functions creates autograd history and calls copy_! Bad! 4. Optimizers did not create buffers after add_parameters was called. Pull Request resolved: pytorch#9837 Reviewed By: goldsborough Differential Revision: D9199746 Pulled By: ebetica fbshipit-source-id: 76d6b22e589a42637b7cc0b5bcd3c6b6662fb299

ebetica requested a review from goldsborough July 25, 2018 19:27

ebetica requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners July 25, 2018 19:27

ebetica force-pushed the virtual_superclass branch from 2ca83fb to 66af4d4 Compare July 25, 2018 19:55

weiyangfb added the ready for review (this tag is deprecated) All PRs are ready for review unless they are draft, WIP, or have undismissed requested changes label Jul 31, 2018

goldsborough approved these changes Aug 1, 2018

View reviewed changes

test/cpp/api/sequential.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

goldsborough force-pushed the virtual_superclass branch from 4e5c14b to 4a0a492 Compare August 7, 2018 01:59

ebetica and others added 8 commits August 7, 2018 11:25

Virtual superclass in Cloneable

058f198

public optimizer options

5e3b8f4

Serialization shouldn't create autograd history

ea681fc

Optimizers correctly create buffers after add_parameters is called

28e6fa9

Update variable.h

dff50da

static_pointer_cast -> dynamic_pointer_cast after rebase

665a983

Dont detach() inside load()

b937924

Change grad() back to Variable

a0f7249

goldsborough force-pushed the virtual_superclass branch from 4a0a492 to a0f7249 Compare August 7, 2018 18:25

facebook-github-bot reviewed Aug 7, 2018

View reviewed changes

ezyang approved these changes Aug 7, 2018

View reviewed changes

ebetica commented Aug 7, 2018

View reviewed changes

Copy data

2bbaa7f

facebook-github-bot reviewed Aug 9, 2018

View reviewed changes

facebook-github-bot reviewed Aug 10, 2018

View reviewed changes

facebook-github-bot closed this in b8530dc Aug 13, 2018

ezyang added the merged label Jun 26, 2019

Conversation

ebetica commented Jul 25, 2018

Uh oh!

goldsborough commented Jul 30, 2018

Uh oh!

ebetica commented Jul 30, 2018

Uh oh!

goldsborough left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

goldsborough commented Aug 7, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

ebetica commented Aug 7, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants