Expose all netstate options (for all-in-one nets) #3863

lukeyeager · Mar 21, 2016

There are 3 configurable dimensions to a network's NetState: phase (TRAIN or TEST), level and stage. Currently, only phase is exposed through common interfaces. This PR exposes level and stage to the user, further enabling all-in-one nets (#1245)

New CLI:

build/tools/caffe time -model network.prototxt -level 1 -stage deploy

New pycaffe interface:

net = caffe.Net('network.prototxt', weights='weights.caffemodel',
  phase=caffe.TEST, level=1, stages=['deploy'])

EDIT: PR has been updated not to break old code

~~This is a breaking change (see #1790 (comment) for an explanation). Users will need to transform pycaffe code like:~~

# old
net = caffe.Net('network.prototxt', 'weights.caffemodel', caffe.TEST)

~~to:~~

# new
net = caffe.Net('network.prototxt', weights='weights.caffemodel', phase=caffe.TEST)
# or
net = caffe.Net('network.prototxt', caffe.TEST, weights='weights.caffemodel')

With some effort, this could probably be done without breaking the interface (but I'd need some help with the Boost.Python code). However, I am actually in favor of breaking the interface in order to make it simpler and more "Pythonic".

seanbell · Mar 25, 2016

While I agree that the new interface is more Pythonic, I personally think that the old interface can be maintained with a little bit of massaging. All that needs to be edited is the Net.__init__ function, which can be done in the same style as the rest of the _Net_X functions.

Modify the end of pycaffe.py by adding:

# Original Boost.Python Net.__init__ function
_Net_init_boost = Net.__init__


def _Net_init(self, *args, **kwargs):
    """ Construct a caffe.Net """
    # Transform the arguments to handle the legacy API
    if len(args) == 2 and not kwargs:
        # Handle case: Net(param_file, phase)
        _Net_init_boost(self, args[0], weights_file=None, phase=args[1])
    elif len(args) == 3 and not kwargs:
        # Handle case: Net(param_file, weights_file, phase)
        _Net_init_boost(self, args[0], weights_file=args[1], phase=args[2])
    else:
        # Handle new API:
        # Net('net.prototxt', weights_file='weights.caffemodel', phase=caffe.TEST, level=1, stages=['deploy'])
        _Net_init_boost(self, *args, **kwargs)


# Attach methods to Net.
Net.__init__ = _Net_init
...

The above code makes make pytest pass, but I haven't tested it thoroughly.

One other note: It seems that if you wanted to use a kwarg for every argument, then in some place it's named param_file and in others it's named network_file. Maybe this should be harmonized to use the same name?

Could you also insert the new arguments (level, stages) after root_net? Otherwise, this breaks calling code that instantiates Net with root_net non-null:

Net(param_file, phase, root_net);

I'm not sure how common this is, but it's pretty trivial to avoid it by just inserting the new optional arguments at the end of the declaration?

I agree, that seems like the safer choice. I wonder why no tests failed? Seems like a test hole.

Does anyone use the root_net parameter? I'd like to keep the phase, level and stages parameters next to each other since they're naturally grouped. If anything, I'd prefer to move root_net before phase, but that would definitely break existing code.

As per Evan's comments below, I reverted back to the original behavior of moving root_net to the end.

lukeyeager · Apr 18, 2016

One other note: It seems that if you wanted to use a kwarg for every argument, then in some place it's named param_file and in others it's named network_file. Maybe this should be harmonized to use the same name?

I agree, the naming change is annoying. network_file and weights_file seem more meaningful to me than param_file and pretrained_param_file. They do a better job of conveying the fact that the first file is a textual representation of the network, while the second file is a binary file containing numerical weights.

I thought it might be appropriate to rename them here since this is the transition from internal code to external interface. Other common interfaces use words like solver.net=... and caffe -weights=....

EDIT - Oh, I see what you were pointing out. I was being internally inconsistent with my own code. Whoops!

lukeyeager · Apr 18, 2016

While I agree that the new interface is more Pythonic, I personally think that the old interface can be maintained with a little bit of massaging. All that needs to be edited is the Net.__init__ function, which can be done in the same style as the rest of the _Net_X functions.

@seanbell I think I've found a nice compromise between our solutions. I've left the old Net_Init_Load() function in place, and I'm using Boost.Python's pattern matching to distinguish between Net(string, string, int) and Net(string, int, int).

Now all of these constructors work as expected:

### Legacy style
net = caffe.Net('network.prototxt', caffe.TRAIN)
net = caffe.Net('network.prototxt', 'weights.caffemodel', caffe.TEST)
  # pattern matches to Net(network_file, weights_file, phase)
  # prints deprecation warning

### New style
net = caffe.Net('network.prototxt', phase=caffe.TRAIN)
net = caffe.Net('network.prototxt', weights_file='weights.caffemodel', phase=caffe.TEST)
net = caffe.Net('network.prototxt', phase=caffe.TEST, weights_file='weights.caffemodel')
net = caffe.Net('network.prototxt', caffe.TRAIN, 0)
  # pattern matches to Net(network_file, phase, level)
net = caffe.Net('network.prototxt', caffe.TEST, 0, [], 'weights.caffemodel')

Made changes and rebased to address merge conflict from https://github.com/BVLC/caffe/pull/3982.

lukeyeager · Apr 26, 2016

Is there anything in particular holding back this PR?

Does the re-arranging of the root_net parameter break any of the @BVLC team's crazy [cool] workflows?

Is my Python constructor compromise not satisfactory?

lukeyeager · May 13, 2016

Updated Net constructor to not switch order of arguments - good idea @ajtulloch
Added -phase flag to tools/caffe so that you can time the TEST phase
Rebased to master

ajtulloch · May 14, 2016

LGTM. Cool stuff @lukeyeager!

shelhamer · May 14, 2016

Will review after NIPS deadline 05/20. Thanks for this @lukeyeager and thanks for review @ajtulloch!

No need for XXX -- the comment is clear as it is.

How about weights instead of weights_file? Including _file is more explicit but I like short arg names and every example will be a path to a .caffemodel.

Why are level and stage not honored for caffe train on the command line? I think it's reasonable for command line args to override the definition, just as the -gpu arg takes precedence over solver_mode.

Would we just set the train_state and not override any of the test_states?
https://github.com/BVLC/caffe/blob/rc3/src/caffe/proto/caffe.proto#L135-L136

That is what I had in mind, but perhaps that's counterintuitive too and we should instead catch when these args are given to caffe train and complain. It's reasonable to not support overriding, and my concern is more about silently running a different net than intended.

I've updated the PR to change train_state. That seems reasonably intuitive since the command is "caffe train" after all.

There should be corresponding tests on the core, C++ side since not everything is in Python (though I use plenty of Python myself). Perhaps most of the tests should actually be core tests and then more abbreviated tests could be done through pycaffe.

Done

shelhamer · May 25, 2016

Re: root_net in https://github.com/BVLC/caffe/pull/3863#discussion_r60153570 and https://github.com/BVLC/caffe/pull/3863#discussion_r59980841, this is a pretty obscure arg that should never really be used outside of the framework itself. Furthermore I hope it can be eventually refactored out through further work on the parallel implementation. With that in mind, I think it is reasonable to keep it as the last, optional arg as to not complicate the normal instantiation of Nets. Thoughts?

shelhamer · May 25, 2016

Although I originally voted for net states and these varied ways of expressing them, I'm now of two minds. On one hand phase, level, and stage states make all-in-one definitions possible and potentially make variations easier to express, but on the other these are just more special cases in the definition of nets. That said, they do seem to address a need in practice and since they are in the library they should at least be properly exposed to its interfaces. Thoughts @jeffdonahue @longjon?

While I now use and suggest net spec, net states have their purposes so thanks for extending them throughout the interfaces @lukeyeager.

For merge:

consider arg name weights_file vs. weights
extend caffe train to accept level, stage or decide not to and warn/die if they are given
migrate net state tests to C++ side of testing, or at least add a basic test there to not rely on Python
switch order of args with root_net?

Let me know if there are other concerns.

lukeyeager · May 25, 2016

I've addressed (1) and (2).

(3) Gulp. Time to dive into gtest ...

(4) Don't care

lukeyeager · May 26, 2016

Added a basic C++ test

lukeyeager · May 26, 2016

Ready for review. Do you want more extensive tests? Would you like me to switch the location of the root_net arg? I'll squash whenever you approve.

shelhamer · Jun 1, 2016

@lukeyeager I'd rather switch the order of root_net as to not have the NULL arg for many standard instantiation of Nets, so please go ahead with that.

It would be best to have a C++ test that checks that the layers for do in fact exist, for instance by checking the layer name in the instantiated net (or differentiate the train and val dimensions of the data top for an alternative check). The current test checks that instantiating with stage does not crash, but does not check what is actually instantiated.

Thanks.

shelhamer · Jun 1, 2016

~~@lukeyeager re: root net, make sure to update solver.cpp accordingly:~~ Just kidding, didn't look closely at the constructor.

At some point all of the root solver/root net code needs to be simplified, but it's here for now and not a problem to be solved by this PR

lukeyeager · Jun 1, 2016

make sure to update solver.cpp accordingly:

None of those invocations need updating - they use a different constructor which expects a NetParameter object instead of a filename.

lukeyeager · Jun 1, 2016

I'd rather switch the order of root_net

Done

It would be best to have a C++ test that checks that the layers for do in fact exist

Done

shelhamer · Jun 1, 2016

https://github.com/BVLC/caffe/pull/3863#issuecomment-223065222 Right, sorry about that.

With https://github.com/BVLC/caffe/pull/3863#issuecomment-223070737 this looks good to me. @longjon will double-check for merge.

lukeyeager · Jun 3, 2016

Rebased after conflict from #4227

lukeyeager · Jun 15, 2016

this looks good to me. @longjon will double-check for merge.

ping @longjon

shelhamer · Jul 11, 2016

Merging after offline discussion. Thanks @lukeyeager!

p.s. @jeffdonahue noted that this not strictly necessary since the NetState of the NetParameter can be set before instantiating a Net. However, this PR contributes a one-step interface to instantiating any state and enables exposing the different states to languages lacking protobuf bindings (such as matlab, as far as I know).

lukeyeager · Jul 11, 2016

this not strictly necessary since the NetState of the NetParameter can be set before instantiating a Net.

Sure, but it's nice to be able to change it without editing .prototxt.

Merging after offline discussion.

Thanks!

This was referenced Mar 21, 2016

Closed

Expose net stages to pycaffe #3736

Open

[Discussion] How to specify all-in-one networks #3864

Merged

Give phase to Net #1790

shelhamer added interface JD JL ES labels Mar 22, 2016

shelhamer added the focus label Apr 26, 2016

lukeyeager referenced this pull request May 19, 2016
Closed
Update summarize.py to select different netstate options #4174

shelhamer added ready for review and removed ES JD JL labels Jun 9, 2016

This was referenced Jun 27, 2016

Closed

draw net doesn't understand net phases #4365

Merged

Add phase support for draw net #4408

shelhamer merged commit 3e94c0e into BVLC:master Jul 11, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lukeyeager deleted the lukeyeager:bvlc/expose-all-netstate-options branch Jul 11, 2016

fxbit added a commit to Yodigram/caffe that referenced this pull request Sep 1, 2016

shelhamer + fxbit Merge pull request #3863 from lukeyeager/bvlc/expose-all-netstate-opt…
…ions Expose all netstate options (for all-in-one nets)
b82abb9

@@ -25,6 +25,7 @@ class Net {
       public:
        explicit Net(const NetParameter& param, const Net* root_net = NULL);
        explicit Net(const string& param_file, Phase phase,
+    +      const int level = 0, const vector<string>* stages = NULL,

@@ -237,7 +260,12 @@ BOOST_PYTHON_MODULE(_caffe) {
        bp::class_<Net<Dtype>, shared_ptr<Net<Dtype> >, boost::noncopyable >("Net",
          bp::no_init)
-    -    .def("__init__", bp::make_constructor(&Net_Init))
+    +    // Constructor
+    +    .def("__init__", bp::make_constructor(&Net_Init,
+    +          bp::default_call_policies(), (bp::arg("network_file"), "phase",
+    +            bp::arg("level")=0, bp::arg("stages")=bp::object(),
+    +            bp::arg("weights_file")=bp::object())))
+    +    // XXX Legacy constructor

@@ -237,7 +260,12 @@ BOOST_PYTHON_MODULE(_caffe) {
        bp::class_<Net<Dtype>, shared_ptr<Net<Dtype> >, boost::noncopyable >("Net",
          bp::no_init)
-    -    .def("__init__", bp::make_constructor(&Net_Init))
+    +    // Constructor
+    +    .def("__init__", bp::make_constructor(&Net_Init,
+    +          bp::default_call_policies(), (bp::arg("network_file"), "phase",
+    +            bp::arg("level")=0, bp::arg("stages")=bp::object(),
+    +            bp::arg("weights_file")=bp::object())))

@@ -34,6 +34,13 @@ DEFINE_string(solver, "",
          "The solver definition protocol buffer text file.");
      DEFINE_string(model, "",
          "The model definition protocol buffer text file.");
+    +DEFINE_string(phase, "",
+    +    "Optional; network phase (TRAIN or TEST). Only used for 'time'.");
+    +DEFINE_int32(level, 0,

              os.remove(net_file)
              os.remove(f.name)
              for name in self.net.params:
                  for i in range(len(self.net.params[name])):
                      self.assertEqual(abs(self.net.params[name][i].data
                          - net2.params[name][i].data).sum(), 0)
+    +
+    +class TestLevels(unittest.TestCase):

BVLC/caffe

Join GitHub today

Expose all netstate options (for all-in-one nets) #3863

Conversation

lukeyeager commented Mar 21, 2016 • edited

This was referenced Mar 21, 2016

shelhamer added interface JD JL ES labels Mar 22, 2016

seanbell commented Mar 25, 2016

ajtulloch commented on the diff Apr 17, 2016

lukeyeager commented Apr 18, 2016 • edited

lukeyeager commented Apr 18, 2016

lukeyeager commented Apr 26, 2016

shelhamer added the focus label Apr 26, 2016

lukeyeager commented May 13, 2016

ajtulloch commented May 14, 2016

shelhamer commented May 14, 2016

lukeyeager referenced this pull request May 19, 2016

Show 1 comment Hide comment shelhamer commented on an outdated diff May 25, 2016

shelhamer May 25, 2016 • edited

Show 1 comment Hide comment shelhamer commented on an outdated diff May 25, 2016

shelhamer commented on the diff May 25, 2016

shelhamer commented on the diff May 25, 2016

shelhamer commented May 25, 2016

shelhamer commented May 25, 2016 • edited

lukeyeager commented May 25, 2016

lukeyeager commented May 26, 2016

lukeyeager commented May 26, 2016

shelhamer commented Jun 1, 2016

shelhamer commented Jun 1, 2016 • edited

lukeyeager commented Jun 1, 2016

lukeyeager commented Jun 1, 2016

shelhamer commented Jun 1, 2016

lukeyeager added some commits Jun 1, 2016

lukeyeager commented Jun 3, 2016

shelhamer added ready for review and removed ES JD JL labels Jun 9, 2016

lukeyeager commented Jun 15, 2016

This was referenced Jun 27, 2016

shelhamer commented Jul 11, 2016

Hide details View details shelhamer merged commit 3e94c0e into BVLC:master Jul 11, 2016 1 check passed

1 check passed

lukeyeager commented Jul 11, 2016

lukeyeager deleted the lukeyeager:bvlc/expose-all-netstate-options branch Jul 11, 2016

This was referenced Jul 12, 2016

fxbit added a commit to Yodigram/caffe that referenced this pull request Sep 1, 2016

shelhamer commented on an outdated diff May 25, 2016

shelhamer commented on an outdated diff May 25, 2016

shelhamer merged commit `3e94c0e` into BVLC:master Jul 11, 2016
1 check passed