[Placeholder] Allow the differentiation of Placeholder nodes. #1612
Conversation
nadavrot
force-pushed
the
differentiate_placeholder
branch
2 times, most recently
from
7a97d67 to
32e009e
Compare
nadavrot
requested review from
artemrakhov-glow,
opti-mix,
beicy,
bertmaher and
qcolombet
include/glow/Graph/Nodes.h
Outdated
| @@ -31,8 +31,12 @@ namespace glow { | |||
| // Storage is the base class for Variables, which are bound to tensors, and | |||
| // Placeholder nodes which are unbound. | |||
| class Storage : public Node { | |||
| /// Specifies if the variable is trainable. | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
include/glow/Graph/Nodes.h
Outdated
| @@ -50,6 +54,9 @@ class Storage : public Node { | |||
| Node *clone() const; | |||
| /// @} | |||
|
|
|||
| /// \returns True if the Variable is initialized to be in training mode. | |||
| bool isTraining() const { return isTrainable_; } | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| @@ -144,8 +144,8 @@ class ChildMemSizeBasedScheduler : public Scheduler { | |||
| // We don't model memory dependencies, but we still need to honor them. | |||
| // Make sure the SaveNode happens after the last use of the output variable. | |||
| if (auto *save = dyn_cast<SaveNode>(N)) { | |||
| Variable *output = save->getVariable(); | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| auto *input1 = mod.createPlaceholder(ty, "input1"); | ||
| auto *input2 = mod.createPlaceholder(ty, "input2"); | ||
| auto *input3 = mod.createPlaceholder(ty, "input3"); | ||
| auto *input1 = mod.createPlaceholder(ty, "input1", false); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
|
||
| // Check that the Placeholder has multiple users, because at least one write | ||
| /// node will be added. | ||
| EXPECT_GE(A->getNumUsers(), 1); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Add the isTrain flag to placeholders. As we are working to get rid of mutable variables, this flag will allow us to convert all of our tests from Variable to Placeholder without rewriting all of our training tests.
Allow differentiation of Placeholder variables. This commit fixes a few bugs in the scheduler, cloner and differentiation function, where we assumed that the inputs to the graph are variables, and not placeholders. This commit adds a unit test that checks that we can differentiate (and later compile) functions that differentiate (and update) placeholders.
nadavrot
force-pushed
the
differentiate_placeholder
branch
from
32e009e to
48adbe7
Compare
|
@artemrakhov Yes! That's a good point. Thank you. |
rdzhabarov
changed the title
Description: Allow the differentiation of Placeholder nodes. This PR includes two main parts. First, we add a flag to Placeholder to match the Variable node and allow the differentiation of the selected nodes. The second part fixes a few places (scheduler, cloner, etc) that assume that all storage nodes are Variable.
Testing: Added unit tests and ran ninja check.
Documentation: None.
This is related to #1334. This PR is a step in the direction of eliminating the Variables, and is required for porting our unit tests to Placeholders instead of training mutable variables.