plasmodic / ecto

ecto is a dynamically configurable Directed Acyclic processing Graph (DAG) framework.
http://ecto.willowgarage.com/
BSD 3-Clause "New" or "Revised" License
97 stars 37 forks source link

Directed configuration revisited #264

Closed stonier closed 9 years ago

stonier commented 9 years ago

Previously

Configuration of a plasm occurred on an unsorted graph, i.e. in Plasm::configure_all() we have:

BOOST_FOREACH(impl::ModuleVertexMap::value_type& x, impl_->mv_map)
{
    x.first->configure();
}

Motivation for Directed Configuration

I've been running into use cases though where I'd like the configuration of one cell to help with the configuration of the next cell in a directed way, just like processing does. For example, loading parameters into a CameraInfo object to be used by a pipeline. These parameters can come from a yaml file, from ros params, over the wire and instead of having all the code bundled into the runtime cell, it's useful to have common code dropped into ecto cells such as LoadYamlFromFile, LoadYamlFromRosParam, Yaml2CameraInfo. If these are all configured in a directed graph that passes the eventual object onto the runtime cell...awesome.

There are of course other ways to do this - as mentioned above, you could drop all the code in the runtime cell, or glue in alot of python code in the ectoscript for instance. The first causes cells to become rather borg-like. It also doesn't let you configure via python code. The latter doesn't let you configure using existing c++ libraries and classes. Getting cells to do this lets you reuse code, mix and match python and c++ code where you want...and just feels alot more plasmodic in general.

How

This pull request enables sorting on the graph before the configure method is called for each cell in the graph. It also moves outputs to inputs so configuration in one cell can affect the next cell. Sorting is done in the Plasm::configure_all() step:

    /****************************************
     ** Unsorted Configuration - Deprecated
     ****************************************/
    // BOOST_FOREACH(impl::ModuleVertexMap::value_type& x, impl_->mv_map)
    //      {
    //        x.first->configure();
    //      }
    /****************************************
     ** Sorted Configuration - Depth First
     ****************************************/
    std::vector<ecto::graph::graph_t::vertex_descriptor> stack;
    boost::topological_sort(impl_->graph, std::back_inserter(stack));
    std::reverse(stack.begin(), stack.end());
    BOOST_FOREACH(const ecto::graph::graph_t::vertex_descriptor& vd, stack)
    {
      ecto::graph::invoke_configuration(impl_->graph, vd);
    }
    /****************************************
     ** Sorted Configuration - Breadth First
     ****************************************/
    // Note that the default scheduler uses this instead. Bring it in, if needed.

Moving inputs to outputs reuses the directed processing code.

Side Effects

One side effect of this is that it requires cells to actually initialise outputs in the configuration step if they want to be part of a directed configuration. You can see I've amended the Constant and Dealer cells in this pull request, e.g in Constant.cpp:

    void configure(const ecto::tendrils& params,
                   const ecto::tendrils& /* in */,
                   const ecto::tendrils& out)
    {
      // initialise outputs so it can be used with directed configuration
      *out_ = *value_;
    }

Without this initialisation, tests fail with an error in the configuration step:

[cell_name] = ecto_test::FileO

[type] = boost::exception_detail::clone_impl<ecto::except::ValueNone>

[what] = ValueNone

[when] = Copying out to input

If directed configuration is to be used, we have two options. 1) require output initialisation as a policy or 2) figure out some way to robustify directed configuration so that uninitialised outputs can be ignored

This pull request is adopting 1) for now. 2) is certainly possible - we can pass a flag into the move_inputs and move_outputs function notifying them that it is a configuration step and do not throw on uninitialised variables. However this might be unintuitive when using cells and not knowing if they can be relied on for directed configuration or not.

stonier commented 9 years ago

Note : all tests passing with this.

vrabaud commented 9 years ago

makes sense, and the option 1 is fine by me.