Separate bwi_kr_execution into its own repo

[nickswalker]

kr_execution is going to play a larger role in RoboCup@Home this year so the team would appreciate any efforts to enhance the ease of reuse. Step one would be moving the package into its own repository. This will allow us to avoid maintaining a fork of bwi_common and make eventual upstream contributions easier.

@YuqianJiang has kindly agreed to think about what this will take.

[jack-oquin]

This seems like a good idea to me.

The only obvious problem is the dependency on bwi_msgs. We need to figure out exactly which messages it uses and which packages depend on them.

Also, I'd like to repeat the plea that ROS package names should not contain acronyms. While some people know that "KR" means "knowledge representation", most don't.

[rdelfin]

For reference, the full list of bwi_msgs files used by bwi_kr_execution are:

• QuestionDialog.srv
• DoorHandlerInterface.srv
• LogicalNavigation.action
• ResolveChangeFloor.srv
• RobotTeleporterInterface.srv

[jack-oquin]

Another factor to remember is that the bwi_kr_execution in the master branch only works with the Trusty version of clingo, not the Xenial version. The kinetic branch only works on Xenial, not Trusty.

It is likely possible to create a version that works with both, but that will require some extra effort.

[jack-oquin]

In order to share code between the BWI and RoboCup@Home projects, it will probably be necessary to share bwi_msgs too, since they are the primary interfaces between the various components. That's why most of the messages were grouped together in the first place, to simplify the dependency graph.

That line of thought leads to the idea of a common repository at a lower level than bwi_common. To start with, it would contain bwi_msgs and bwi_kr_execution. There might be other shared packages in the future.

It's too bad that bwi_common has gotten too cumbersome to fulfil that role. It's original purpose was to contain common packages that are not specific to a single platform, like the segbot.

[jack-oquin]

Where is that bwi_common fork?

I am curious to understand the differences. Maybe they will give some clues as to what went wrong.

[YuqianJiang]

The actasp library inside kr_execution is designed to be shared among different platforms. It provides interfaces of reasoners and executors, and deals with the Clingo syntax.

If the goal of the new repo is to keep common components of kr_execution for both platforms, not much outside of that library needs to be shared. The two platforms will have some common domain rules, and a few common action executors (mainly LogicalNavigation actions), but I expect most of them to be different.

Therefore, the only thing we need inside bwi_msgs is LogicalNavigation.action, which kr_execution uses to call logical navigator code. The logical navigator code is in BWI’s segbot repo, and slightly adapted for Robocup@Home.

We also need to share the bwi_planning_common package inside bwi_common.

On Jan 20, 2018, at 08:52, jack-oquin notifications@github.com wrote:

In order to share code between the BWI and RoboCup@Home projects, it will probably be necessary to share bwi_msgs too, since they are the primary interfaces between the various components. That's why most of the messages were grouped together in the first place, to simplify the dependency graph.

That line of thought leads to the idea of a common repository at a lower level than bwi_common. To start with, it would contain bwi_msgs and bwi_kr_execution. There might be other shared packages in the future.

It's too bad that bwi_common has gotten too cumbersome to fulfil that role. It's original purpose was to contain common packages that are not specific to a single platform, like the segbot.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[nickswalker]

Our fork. We also brought over the map annotation tools. @jack-oquin you should have an invite to the Villa org in your email.

I think a common repo is a helpful convenience for internal development, but it presents ergonomic difficulties when people on the outside only need a subset of the packages. Our build times were already too long, so we had to manually prune what we weren't using.

[jack-oquin]

If build time is the problem, they should use the catkin tools profile command. Just because other packages exist in the repository does not require building them all.

There are ten bwi_common packages in that bwi_nav repository. That's going to be a nightmare for ongoing code maintenance. There are non-trivial changes for ROS Kinetic in the main bwi_common. How will they be ported to bwi_nav? There is not even any reasonable way to compare the versions or generate pull requests.

[jack-oquin]

Note that segbot_gui is obsolete. It was replaced by bwi_rqt_plugins, but some of the launch files erroneously still ran the corresponding segbot_gui scripts. That has been fixed in the Kinetic branches, and segbot_gui has been deleted in that branch.

That was an example of a mental error: mixing up segbot-specific code with generic BWI features, which belong in bwi_common.

It looks like segbot_logical_translator is another example of the same error. it is referenced in two of the bwi_kr_execution launch scripts and therefore should have been declared as an <exec_depend>. That dependency is not declared and would not be valid for a segbot repository package. We should definitely clean that up.

xref: #101

[jack-oquin]

The more I look into this issue, the less I like the idea of creating a new repository.

Instead, I favor cleaning up some of the historical cruft in segbot and bwi_common, and changing our build practices to use catkin build and not catkin_make, which mindlessly compiles everything in the workspace whether it's needed or not.

This is an important issue for how we organize our software for ongoing research, and deserves serious discussion.

@justinhart, @jsinapov, @warnellg, @rdelfin: what are your thoughts?

[nickswalker]

Ah, blacklisting packages through catkin tools is a good idea.

The apparent dependencies on packages outside of bwi_common was my second motivation in asking for a dedicated, isolated kr repo. If those can be mitigated otherwise, it would make it much easier to keep the fork clean.

[jack-oquin]

I've been using catkin tools for years, but had previously been reluctant to recommend that others change their work flow away from the familiar catkin_make.

I have recently become an advocate for everyone in our lab switching to the more-modern build tool. It solves several problems for us, while being much more pleasant to use. It is actually required now for building the BWI repositories with Kinetic.

Regarding outside dependencies: bwi_common is not supposed to depend on any of our other repositories. Any such dependency is a bug, and we need to fix it, as described in #101.

[YuqianJiang]

@nickswalker and I looked into this. Instead of creating a new repo for kr_execution, we will separate platform specific code from kr_execution into its own package, namely domain rules and action executors.

[jack-oquin]

Is there a consensus that we don't want to create another repository at this point?

If so, I recommend closing this issue and working on the other related problems under other, more-specific issues.

[nickswalker]

Yes, I think there is.