Add app manager plugin

[knorth55]

Adding app manager plugin

Note

this plugin depends on roslaunch>=1.13.6 ros/ros_comm#1115
UPDATE: I overwrite required function for kinetic users in cb23aae

Description

Now, I'm planning to add plugins for app_manager.
This is because I want to use app information and result before and after the app.
This PR is draft and proposal for app_manager plugins, and I made app_manager plugin examples in https://github.com/knorth55/app_manager_utils.
I made these plugins below for apps.

• data recorder plugin
  • video recorder plugin: record image during an app
  • rosbag recorder plugin: record rosbag during an app
• data uploader plugin
  • google drive uploader plugin: upload recorded data in google drive
• notifier plugin
  • mail notifier: mail app results and uploaded data url

How to make new plugin

Add launch

First, we can set launch file for plugin as normal launch as belows.

<launch>
  <!-- launch_args: arguments for plugin launch -->
  <arg name="foo" />
  <!-- add some nodes  -->
  <node name="$(anon rosbag_recorder)" pkg="rosbag" type="record" args="/tf /joint_states" />
</launch>

Add plugin module

Then, we need to set plugin function as follows.

class RosbagRecorderPlugin(object):
    # app_manager plugin function before app
    # app: app definition
    # ctx: app context which is shared through all plugins
    # plugin_args: plugin arguments defined in app file
    @classmethod
    def app_manager_start_plugin(cls, app, ctx, plugin_args):
        # do something before app runs
        return ctx

    # app_manager plugin function after app
    # app: app definition
    # ctx: app context which is shared through all plugins
    # plugin_args: plugin arguments defined in app file
    @classmethod
    def app_manager_stop_plugin(cls, app, ctx, plugin_args):
        # do something after app runs
        return ctx

Add plugin yaml

Next, we need to add plugin config yaml file as follows.

- name: app_recorder/rosbag_recorder_plugin  # plugin name
  launch: app_recorder/rosbag_recorder.launch  # plugin launch name
  module: app_recorder.rosbag_recorder_plugin.RosbagRecorderPlugin  # plugin module name

In package.xml, we need to set yaml file path then as follows.

  <export>
    <app_manager plugin="${prefix}/app_recorder_plugin.yaml" />
  </export>

How to enable plugin in an app

In order to enable plugins, we need to write plugins in app file (i.e. test_app.app) as follows.

plugins: # plugin definitions
  - name: mail_notifier_plugin  # name to identify this plugin
    type: app_notifier/mail_notifier_plugin  # plugin type
    launch_args:  # arguments for plugin launch file
      - foo: hello
    launch_arg_yaml: /etc/mail_notifier_launch_arg.yaml  # argument yaml file for plugin launch file
    # in this case, these arguments will be passed.
    # {"hoge": 100, "fuga": 30, "bar": 10} will be passed to start plugin
    # {"hoge": 50, "fuga": 30} will be passed to stop plugin
    plugin_args:  # arguments for plugin function
      - hoge: 10
      - fuga: 30
    start_plugin_args:  # arguments for start plugin function
      - hoge: 100  # arguments for start plugin function arguments (it overwrites plugin_args hoge: 10 -> 100)
      - bar: 10
    stop_plugin_args:  # arguments for stop plugin function
      - hoge: 50  # arguments for stop plugin function arguments (it overwrites plugin_args hoge: 10 -> 50)
    plugin_arg_yaml: /etc/mail_notifier_plugin_arg.yaml  # argument yaml file for plugin function arguments
  - name: rosbag_recorder_plugin  # another plugin
    type app_recorder/rosbag_recorder_plugin
    launch_args:
      rosbag_path: /tmp
      rosbag_title: test.bag
      compress: true
      rosbag_topic_names:
        - /rosout
        - /tf
        - /tf_static
plugin_order: # plugin running orders. if you don't set field, plugin will be run in order in plugins field
  start_plugin_order:  # start plugin running order
    - rosbag_recorder_plugin  # 1st plugin name
    - mail_notifier_plugin  #2nd plugin name
  stop_plugin_order:  # start plugin running order
    - rosbag_recorder_plugin
    - mail_notifier_plugin

[v4hn]

I did not read through the code, but some feedback on the idea described in the description.

class RosbagRecorderPlugin(object):

Does it make sense to provide a base class (with unimplemented interface functions)?

Is there a default python plugin that does "nothing" if nothing is specified in the config?

From what I can see the new python layer is independent from the rest of the plugin, as the launch file defines the "actual" plugin. How about renaming your python wrappers from module to loader or context and keeping the names independent from specific plugins as long as they do not do specific things that can only ever be relevant for one plugin?

[knorth55]

@v4hn thank you for your comment.

Does it make sense to provide a base class (with unimplemented interface functions)?

in my repository, I made the base class.
https://github.com/knorth55/app_manager_utils/blob/master/app_manager_plugin/src/app_manager_plugin/app_manager_plugin_base.py
is it better to provide it in this repo?

From what I can see the new python layer is independent from the rest of the plugin, as the launch file defines the "actual" plugin. How about renaming your python wrappers from module to loader or context and keeping the names independent from specific plugins as long as they do not do specific things that can only ever be relevant for one plugin?

I'm not sure about your comment, but you are thinking about a plugin whose python layer does not do anything, right?
In that case, you can write as follow.
but maybe it is better to support null (or None) for skip running the python layer.

- name: app_recorder/rosbag_recorder_plugin  # plugin name
  launch: app_recorder/rosbag_recorder.launch  # plugin launch name
  module: app_manager_plugin.app_manager_base.AppManagerPlugin

[v4hn]

is it better to provide it in this repo?

I didn't look at it, but it seems reasonable to me to define an abstract interface? It's python though, and I don't know about best practices in the ROS python community for things like that...

but maybe it is better to support `null (or None)` for skip running the python layer.

Yes! Wouldn't you otherwise break old configurations for no reason?

[knorth55]

I didn't look at it, but it seems reasonable to me to define an abstract interface?
It's python though, and I don't know about best practices in the ROS python community for things like that...

OK, i will

Yes! Wouldn't you otherwise break old configurations for no reason?

this plugin PR (I mean current proposal) does not break old configurations.
App configuration without plugin description skip all plugins.
https://github.com/knorth55/app_manager/blob/add-app-manager-plugin/src/app_manager/app_manager.py#L308

[knorth55]

@v4hn
Thank you for your review.
I update PR to follow your advice.

Are mail_notifier_plugin and rosbag_recorder_plugin available somewhere?
I would be ok with a JSK reference somewhere.

I made app_manager_utils repo: https://github.com/knorth55/app_manager_utils
in this repo, we have these plugins

• app_recorder
  • rosbag_recorder_plugin: Record rosbag during app
  • video_recorder_plugin: Record video during app
• app_notifier
  • mail_notifier_plugin: Send e-mail about app results
  • speech_notifier_plugin: Speak about app results
• app_uploader
  • `gdrive_uploader_plugin: Upload files to Google Drive

If we add reference to this repo, where can I add the url link?

[knorth55]

I update this PR to add some new features:

plugin_order: plugin running order, you can define it with plugin name

• start_plugin_order: start plugin running order
• stop_plugin_order: stop plugin running order

[start/stop]_plugin_args: arguments only for start/stop plugin

• start_plugin_args: arguments for start plugin function (only start, not stop plugin)
• stop_plugin_args: arguments for stop plugin function (only stop, not start plugin)

[start/stop]_plugin_args have higher priority than plugin_args, which means plugin_args is overwritten if it has same key.

load arguments from yaml file

• launch_arg_yaml: argument yaml file for plugin launch file (you can define launch_args in another file and load it)
• plugin_arg_yaml: argument yaml file for plugin function (you can define plugin_args in another file and load it).
• start_plugin_arg_yaml: argument yaml file for start_plugin function (you can define start_plugin_args in another file and load it).
• stop_plugin_arg_yaml: argument yaml file for stop_plugin function (you can define stop_plugin_args in another file and load it).

if yaml file and arguments in app have same key, app_manager shows error.

Example

plugins: # plugin definitions
  - name: mail_notifier_plugin  # name to identify this plugin
    type: app_notifier/mail_notifier_plugin  # plugin type
    launch_args:  # arguments for plugin launch file
      - foo: hello
    launch_arg_yaml: /etc/mail_notifier_launch_arg.yaml  # argument yaml file for plugin launch file
    # in this case, these arguments will be passed.
    # {"hoge": 100, "fuga": 30, "bar": 10} will be passed to start plugin
    # {"hoge": 50, "fuga": 30} will be passed to stop plugin
    plugin_args:  # arguments for plugin function
      - hoge: 10
      - fuga: 30
    start_plugin_args:  # arguments for start plugin function
      - hoge: 100  # arguments for start plugin function arguments (it overwrites plugin_args hoge: 10 -> 100)
      - bar: 10
    stop_plugin_args:  # arguments for stop plugin function
      - hoge: 50  # arguments for stop plugin function arguments (it overwrites plugin_args hoge: 10 -> 50)
    plugin_arg_yaml: /etc/mail_notifier_plugin_arg.yaml  # argument yaml file for plugin function arguments
  - name: rosbag_recorder_plugin  # another plugin
    type app_recorder/rosbag_recorder_plugin
    launch_args:
      rosbag_path: /tmp
      rosbag_title: test.bag
      compress: true
      rosbag_topic_names:
        - /rosout
        - /tf
        - /tf_static
plugin_order: # plugin running orders. if you don't set field, plugin will be run in order in plugins field
  start_plugin_order:  # start plugin running order
    - rosbag_recorder_plugin  # 1st plugin name
    - mail_notifier_plugin  #2nd plugin name
  stop_plugin_order:  # start plugin running order
    - rosbag_recorder_plugin
    - mail_notifier_plugin

[knorth55]

@v4hn
If you have time, can you review this PR?
What should I do for the improvement?

[v4hn]

@knorth55 I reviewed this multiple times and then you suddenly added a bunch of additional features that (optionally) complicate the configuration even more... That was not a good move w.r.t. reviewer commitment. :-)

load arguments from yaml file

This seems very much superfluous to me. If a plugin really needs arbitrary yaml input (instead of well-defined parameters) they can always add another parameter to their launch file to ask for a yaml file.
The structure of your plugin configuration is already damn complex for anyone but yourself! 😆

Aside from that I approve once more.

Please get @k-okada to do a second review to get this merged.
I expect this is used in your lab so he should be interested.

If you have time

Muhahaha, as if I'd ever...

[v4hn]

If we add reference to this repo, where can I add the url link?

Please add a documentation of this plugin feature to the README.md file in this repository.
You can also give the links there.

[knorth55]

@v4hn

I reviewed this multiple times and then you suddenly added a bunch of additional features that (optionally) complicate the configuration even more... That was not a good move w.r.t. reviewer commitment. :-)

Sorry for that. Now I changed this PR from draft to open.
I will no more change this drastically.

This seems very much superfluous to me. If a plugin really needs arbitrary yaml input (instead of well-defined parameters) they can always add another parameter to their launch file to ask for a yaml file.

I'm sorry again for this change.
The reason why I need this feature is that I want to load import information which should not be managed by git.
Yaml file is useful to load important information like e-mail address.
I want to support both previous and yaml one.

Anyway, thank you for reviewing this again!

Muhahaha, as if I'd ever...

😆

[knorth55]

@v4hn

Please add a documentation of this plugin feature to the README.md file in this repository.
You can also give the links there.

Thanks, I will add it.

[v4hn]

The reason why I need this feature is that I want to load import information which should not be managed by git.

Sounds reasonable, though it adds more complexity. 👍

[knorth55]

Please add a documentation of this plugin feature to the README.md file in this repository.
You can also give the links there.

@v4hn sorry for the lateness, but i update this PR to add readme documentation.

[v4hn]

Merging, as the only other maintainer @k-okada did not comment here at all despite several pings.

Thanks for all the iterations on this @knorth55 !

[knorth55]

@v4hn thank you for reviewing many times and merging this PR!