Skip to content

Comments

Support arbitrary DAGs with SplitLayer#114

Closed
jeffdonahue wants to merge 19 commits intoBVLC:masterfrom
jeffdonahue:dags-by-split
Closed

Support arbitrary DAGs with SplitLayer#114
jeffdonahue wants to merge 19 commits intoBVLC:masterfrom
jeffdonahue:dags-by-split

Conversation

@jeffdonahue
Copy link
Contributor

Addresses #92 as per Yangqing's suggested implementation using SplitLayers. Basically, this allows any blob to be used as "bottom"s multiple times in a network definition.

My opinion is that in the long run it would be better to do this in a less wasteful manner by changing the specification of Backward(...) to add to the current _diff, but this wasteful implementation is better than simply not supporting a blob shared as input by multiple layers.

@sguada
Copy link
Contributor

sguada commented Feb 15, 2014

Would be possible to reuse the blobs as in dropout or relu layers?

@jeffdonahue
Copy link
Contributor Author

Good point Sergio; you can make the 0th top blob in-place by giving it the same name as the bottom blob, but the remaining top blobs can't be done in-place as they need their own diffs (at least, I think..). Implemented this behavior in the commits since your comment.

@sguada
Copy link
Contributor

sguada commented Feb 16, 2014

It looks very good Jeff, I'll look into more detail tomorrow.
Maybe we could avoid creating split layers for those that don't require
backward computation, like data.
One more test in a case that a layer has two top which are used by multiple
layers will ge great.

Sergio
On Feb 15, 2014 4:35 PM, "Jeff Donahue" notifications@github.com wrote:

Good point Sergio; you can make the 0th top blob in-place by giving it the
same name as the bottom blob, but the remaining top blobs can't be done
in-place as they need their own diffs. Implemented this behavior in the
commits since your comment.

Reply to this email directly or view it on GitHubhttps://github.com//pull/114#issuecomment-35173054
.

@jeffdonahue
Copy link
Contributor Author

Good suggestion -- added such a test (TestInsertionTwoTop)

@shelhamer
Copy link
Member

Looks good to me! @Yangqing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thought :)

@Yangqing
Copy link
Member

Looks great to me :)

@shelhamer
Copy link
Member

Merged! We all like DAGs, don't we?

@shelhamer shelhamer closed this Feb 17, 2014
shelhamer added a commit that referenced this pull request Feb 17, 2014
Support arbitrary DAGs with SplitLayer
@aravindhm
Copy link

I found a bug.
Consider this network:-
[datalayer] -> data, label
[convolutional layer] data -> conv1
[relu] conv1 -> conv1 (in-place)
[l1norm as regularizer] conv1 ->
[convolutional layer] conv1 -> reconstruction
[euclidean loss layer] data, reconstruction -> loss

As the regularizer and l1 norm layer are both using conv1, a split layer is inserted. This layer gives conv1 as one of its outputs in-place and thus the network has two layers (relu) and conv1_split both generating conv1 blob.
Error: Duplicate blobs produced by multiple sources.

@forresti
Copy link
Contributor

@aravindhm
I'm having the same problem. I'm using the stock "alexnet" network, and I get this output when running setup:

E0217 16:48:43.778092 7087 test_net.cpp:28] Using GPU
I0217 16:48:43.779001 7087 net.cpp:70] Creating Layer data
I0217 16:48:43.779016 7087 net.cpp:105] data -> data
I0217 16:48:43.779039 7087 net.cpp:105] data -> label
I0217 16:48:43.779091 7087 data_layer.cpp:132] Opening leveldb imagenet-val-leveldb
I0217 16:48:43.872266 7087 data_layer.cpp:169] output data size: 50,3,227,227
I0217 16:48:43.872308 7087 data_layer.cpp:186] Loading mean file frommean.binaryproto
I0217 16:48:43.885260 7087 net.cpp:120] Top shape: 3 227 227
I0217 16:48:43.885293 7087 net.cpp:120] Top shape: 1 1 1
I0217 16:48:43.885301 7087 net.cpp:146] data does not need backward computation.
I0217 16:48:43.885350 7087 net.cpp:70] Creating Layer conv1
I0217 16:48:43.885359 7087 net.cpp:80] conv1 <- data
I0217 16:48:43.885385 7087 net.cpp:105] conv1 -> conv1
I0217 16:48:43.885431 7087 net.cpp:120] Top shape: 96 55 55
I0217 16:48:43.885442 7087 net.cpp:141] conv1 needs backward computation.
I0217 16:48:43.885454 7087 net.cpp:70] Creating Layer conv1_split
I0217 16:48:43.885473 7087 net.cpp:80] conv1_split <- conv1
I0217 16:48:43.885480 7087 net.cpp:94] conv1_split -> conv1 (in-place)
I0217 16:48:43.885488 7087 net.cpp:105] conv1_split -> conv1_split_1
I0217 16:48:43.885507 7087 net.cpp:120] Top shape: 96 55 55
I0217 16:48:43.885521 7087 net.cpp:120] Top shape: 96 55 55
I0217 16:48:43.885530 7087 net.cpp:141] conv1_split needs backward computation.
I0217 16:48:43.885540 7087 net.cpp:70] Creating Layer relu1
I0217 16:48:43.885547 7087 net.cpp:80] relu1 <- conv1
I0217 16:48:43.885555 7087 net.cpp:94] relu1 -> conv1 (in-place)
I0217 16:48:43.885563 7087 net.cpp:120] Top shape: 96 55 55
I0217 16:48:43.885579 7087 net.cpp:141] relu1 needs backward computation.
I0217 16:48:43.885594 7087 net.cpp:70] Creating Layer conv1_split
I0217 16:48:43.885607 7087 net.cpp:80] conv1_split <- conv1
I0217 16:48:43.885622 7087 net.cpp:94] conv1_split -> conv1 (in-place)
F0217 16:48:43.885640 7087 net.cpp:102] Duplicate blobs produced by multiple sources.
*** Check failure stack trace: ***
Aborted (core dumped)

@jeffdonahue
Copy link
Contributor Author

Thanks for the report, this actually breaks any architecture that uses in-place computation, sorry about that :( I'm working on a fix and Evan is going to roll back the change in the meantime.

@forresti
Copy link
Contributor

Thanks, Jeff!

The "arbitrary split" thing is a great idea... I bet you'll be able to work out the kinks. :)

Let me know when we've rolled back to something stable?

shelhamer added a commit that referenced this pull request Feb 18, 2014
Revert dags-by-split merge because split layers were not quite ready.
Sorry for the bug!

We are adopting a `master`/`dev` split development
model, where new commits and pull requests will be merged to `dev` for
testing before integration to `master`. This will keep master clean and
stable.

This reverts commit d339d24, reversing
changes made to 5519826.
@shelhamer
Copy link
Member

Fixed by revert at dda8978. Sorry for the breakage!

We're adopting a split development model of master/dev to prevent such breakage in the future. New commits and pull requests should be made against BVLC/caffe's dev branch. Integration into master will follow after testing and review.

Split layers will be merged into dev once the bugfix is ready.

@sguada
Copy link
Contributor

sguada commented Jun 7, 2014

@jeffdonahue @Yangqing
Do you know if the order in the prototxt is relevant to define a DAG?
Can one layer defining a top that is required by another layer as bottom be defined later in the prototxt?

@jeffdonahue
Copy link
Contributor Author

The order is indeed relevant -- anytime a bottom is named, that name must have been defined previously as a top (so "no" is the answer to your second question).

I considered not requiring this (i.e. allowing non-topological orderings in the prototxt), but decided against it given the added complexity and the fact that it doesn't actually add any extra power (given that a DAG always has a topological ordering). And given that it doesn't add power, one could argue that it's only a hazardous feature, as requiring a topological ordering makes it more likely we'll catch bugs in the net definition not matching the user's intent, and makes the meaning of the net definition clearer to probably any human reader (unless there are weird cases I'm not considering).

I think it'd be strictly better if there were some sort of error message actually indicating the problem in the non-topological ordering case (e.g. crashing with the message A top blob named '____' must be defined before a bottom blob named '____'; you may need to reorder the layers in your network definition. instead of Unknown blob '____' or whatever it is now).

It'd arguably be better (adding flexibility, but also perhaps adding more potential for bugs) to just allow non-topological orderings. I don't really have that much of a preference either way.

mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
Support arbitrary DAGs with SplitLayer
mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
Revert dags-by-split merge because split layers were not quite ready.
Sorry for the bug!

We are adopting a `master`/`dev` split development
model, where new commits and pull requests will be merged to `dev` for
testing before integration to `master`. This will keep master clean and
stable.

This reverts commit d339d24, reversing
changes made to 5519826.
thatguymike pushed a commit to thatguymike/caffe that referenced this pull request Mar 11, 2016
Promote caffe-0.14 branch to master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants