Fix migration rule #1011

efernandez · 2017-03-02T16:43:35Z

This PR is on top of #1010 and fixes the test provided there.

This also replaces #996

efernandez · 2017-03-08T01:28:04Z

@dirk-thomas FYI I've locally seen this unrelated test (https://github.com/ros/ros_comm/blob/lunar-devel/test/test_rosbag/test/test_bag.py#L328) failing intermittently:

$ make run_tests_test_rosbag_nosetests_test.test_bag.py
-- run_tests.py: execute commands
  /usr/bin/cmake -E make_directory /home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag
  /usr/bin/nosetests-2.7 -P --process-timeout=60 /home/eperdomo/dev/ws/clearpath_ws/src/ros_comm/test/test_rosbag/test/test_bag.py --with-xunit --xunit-file=/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag/nosetests-test.test_bag.py.xml
 /tmp/test_rosbag_filter__2.bag                                                                                                               100%              6.2 KB 00:00    
 /tmp/tmpsCAnH2/test_rosbag_filter__2.bag                                                                                                     100%              7.5 KB 00:00    
 /tmp/tmpsCAnH2/test_rosbag_filter__3.bag                                                                                                     100%              7.5 KB 00:00    
...
======================================================================
FAIL: test_get_compression_info (test_bag.TestRosbag)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/eperdomo/dev/ws/clearpath_ws/src/ros_comm/test/test_rosbag/test/test_bag.py", line 328, in test_get_compression_info
    self.assertGreater(info.compressed, 900)
AssertionError: 900 not greater than 900

----------------------------------------------------------------------
Ran 18 tests in 4.223s

FAILED (failures=1)
-- run_tests.py: verify result "/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag/nosetests-test.test_bag.py.xml"
Built target run_tests_test_rosbag_nosetests_test.test_bag.py

$ make run_tests_test_rosbag_nosetests_test.test_bag.py
-- run_tests.py: execute commands
  /usr/bin/cmake -E make_directory /home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag
  /usr/bin/nosetests-2.7 -P --process-timeout=60 /home/eperdomo/dev/ws/clearpath_ws/src/ros_comm/test/test_rosbag/test/test_bag.py --with-xunit --xunit-file=/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag/nosetests-test.test_bag.py.xml
 /tmp/test_rosbag_filter__2.bag                                                                                                               100%              6.2 KB 00:00    
 /tmp/tmpGOevHQ/test_rosbag_filter__2.bag                                                                                                     100%              7.5 KB 00:00    
 /tmp/tmpGOevHQ/test_rosbag_filter__3.bag                                                                                                     100%              7.5 KB 00:00    
...
----------------------------------------------------------------------
Ran 18 tests in 4.221s

OK
-- run_tests.py: verify result "/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test_results/test_rosbag/nosetests-test.test_bag.py.xml"
Built target run_tests_test_rosbag_nosetests_test.test_bag.py

These are two consecutive runs. I've seen values as low as 889. Let me know if you want me to reduce the lower bound to 888.

Either way, this and the others tests pass for this PR.

This allows to check for the following case: Consider the following scenario for a message G with the following versions: G0 -- rule.bmr --> G1 -- auto-generated rule --> G3 where G3 is the current msg, and G2 is the bag message that needs migration. We need to check for a migration rule like this first: G2 --> G3 instead of the backwards ones that were generated before this bug fix: G2 --> G1 Note that the lazy check to mark the rules as valid in the make_update_rule (see https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L1123) method makes that rule pass initially, but later it fails because there's no rule for the sub fields. Here, in test_rosbag, the messages that corresponds to G in the example above, is MessageMixed.

fixes CORE-6641 Consider the following scenario for a message G with the following versions: G0 -- rule.bmr --> G1 -- auto-generated rule --> G3 where G3 is the CURRENT msg, and G2 is the BAG message that needs migration. We need to check for a migration rule like this first: G2 --> G3 instead of the backwards ones that were generated before this bug fix: G2 --> G1 Note that the 'lazy' check to valid the rules in the make_update_rule method makes that rule pass initially, but later it fails because there's no rule for the sub fields.

refs CORE-6641 Checking the sub rules are also valid we allow to check for different positions in the chain for the BAG message we need to migrate, because we don't really know where it should go, and the 'lazy' check for valid in the make_update_rule method makes the suggested rules pass almost always, so we can easily get a rule in the wrong place that later would fail because there are no rules for the sub fields.

efernandez · 2017-03-15T21:26:28Z

Now that #1009 is merged, I've just rebased both #1010 and #1011 on top of lunar-devel, so they can be reviewed.

I recommend reviewing the test on #1010 and here only the two latest commits, which only bring a fairly minimal change compared to the test.
Remember that #1010 is up only for reviewing and showing the current logic fails with the new test. We should close that PR after the review and merge only #1011.

Thanks!

dirk-thomas · 2017-03-17T22:39:57Z

@efernandez This looks good to me. Does it make sense to squash the last two commits?

@mikepurvis What do you think? Did you run this code as part of your testing?

efernandez · 2017-03-20T17:10:52Z

@dirk-thomas I think they cover two different things:

045b7b1 is the main fix for the new test case. It checks for a path directly to the last message first.
bb71941 adds a premature check for sub-rules, so it allows to check more options if the sub-rules fail. Before, the first option with the top rules valid was picked, regardless of the sub-rules. With this commit, the premature check allows to discard options that would fail due to the sub-rules, and therefore check other options. This is an improvement in the line of this comment: https://github.com/clearpathrobotics/ros_comm/blob/bb71941a808dea0a7a9f11960818a00b4a8630db/tools/rosbag/src/rosbag/migration.py#L307

Regarding testing, @mikepurvis cherry-picked the last two commits (not the test stuff because it wasn't implemented by that time) and get it into the bundles we run on our robots https://github.com/ros/ros_comm/commits/lunar-edge (that has this commit 4c4358f, with the two fix commits from here). That's been running now for approximately 1 month with a message that needs the migration fix from this PR. Migration is done on the fly using the rosbag migration (Python) API.

dirk-thomas · 2017-03-20T17:19:44Z

@efernandez Thanks for the explanation. Sounds good to me.

dirk-thomas · 2017-03-20T17:22:47Z

Would you like to see these changes (including the ones from the previous PR) backported (and if yes, wo which ROS distros) or do you not mind since you use a fork anyway?

mikepurvis · 2017-03-20T17:25:42Z

We are running from lunar-devel/lunar-edge now, so no need for backport from our side.

efernandez · 2017-03-20T20:37:12Z

Exactly, no need to backport.

And regarding public ROS packages with changed messages, there are only two AFAIK. One of them is this one:

trajectory_msgs$ l rules/
JointTrajectoryPoint_Groovy2Hydro_08.10.2013.bmr

This was referenced Mar 2, 2017

Fix ROS message migration rule tool #996

Closed

Add test migration rule #1010

Merged

efernandez force-pushed the fix_migration_rule branch from 92f45ee to 4a52cca Compare March 6, 2017 16:42

efernandez mentioned this pull request Mar 6, 2017

Fix test generation #1009

Merged

efernandez force-pushed the fix_migration_rule branch from 4a52cca to 80db563 Compare March 7, 2017 23:01

efernandez force-pushed the fix_migration_rule branch from 80db563 to 75e11de Compare March 9, 2017 00:08

mikepurvis added the hitlist label Mar 14, 2017

Enrique Fernandez added 3 commits March 15, 2017 17:20

efernandez force-pushed the fix_migration_rule branch from 75e11de to bb71941 Compare March 15, 2017 21:21

dirk-thomas merged commit af577c0 into ros:lunar-devel Mar 20, 2017

efernandez deleted the fix_migration_rule branch March 20, 2017 20:32

dirk-thomas mentioned this pull request Oct 26, 2017

changes between 1.12.7 and 1.13.4 for backporting #1205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix migration rule #1011

Fix migration rule #1011

efernandez commented Mar 2, 2017 •

edited

Loading

efernandez commented Mar 8, 2017

efernandez commented Mar 15, 2017

dirk-thomas commented Mar 17, 2017

efernandez commented Mar 20, 2017 •

edited

Loading

dirk-thomas commented Mar 20, 2017

dirk-thomas commented Mar 20, 2017

mikepurvis commented Mar 20, 2017

efernandez commented Mar 20, 2017

Fix migration rule #1011

Fix migration rule #1011

Conversation

efernandez commented Mar 2, 2017 • edited Loading

efernandez commented Mar 8, 2017

efernandez commented Mar 15, 2017

dirk-thomas commented Mar 17, 2017

efernandez commented Mar 20, 2017 • edited Loading

dirk-thomas commented Mar 20, 2017

dirk-thomas commented Mar 20, 2017

mikepurvis commented Mar 20, 2017

efernandez commented Mar 20, 2017

efernandez commented Mar 2, 2017 •

edited

Loading

efernandez commented Mar 20, 2017 •

edited

Loading