Repository: robotika/osgar

Issue:
`application` argument should be string
Where we supply `application` as an argument to a function (like record, replay...) we should supply a string instead, that can be used with `get_class_by_name`. With this change we can replay logs using only generic tools and we can drop the replay capability from i.e. subt/main.py.

It will improve record as well, although not as much since we have custom command line arguments. But it is a step in the right direction anyway.

Discussion:
m3d: Is this code backwards compatible (i.e. if I want to replay old `osgar.go` logfiles)?

```
python -m osgar.replay --module app go-kloubak2-subt-estop-lora-191007_153520.log
original args: b"['/home/robot/git/osgar/osgar/go.py', 'run', 'config/kloubak2-subt-estop-lora.json', '--dist', '10', '--timeout', '120', '--note', 'via sh script']"
stream names:
  app.desired_speed
  app.pose2d
  app.artf_xyz
  app.pose3d
  detector.artf
  detector.dropped
  kloubak.can
  kloubak.encoders
  kloubak.emergency_stop
  kloubak.pose2d
  can.can
  can.raw
  serial.raw
  lidar.raw
  lidar.scan
  lidar_tcp.raw
  lidar_back.raw
  lidar_back.scan
  lidar_back_tcp.raw
  camera.raw
  camera_back.raw
  imu.orientation
  imu.rotation
  imu_serial.raw
  estop.raw
  estop.emergency_stop
  estop_serial.raw
  lora.raw
  lora.cmd
  lora_serial.raw
inputs: ['encoders', 'scan', 'scan_back', 'emergency_stop', 'pose2d', 'buttons', 'voltage', 'rot']
outputs: ['desired_speed', 'pose2d', 'artf_xyz', 'pose3d']
Traceback (most recent call last):
  File "C:\Tool\WinPython-64bit-3.6.2.0Qt5\python-3.6.2.amd64\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "C:\Tool\WinPython-64bit-3.6.2.0Qt5\python-3.6.2.amd64\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "c:\admin\_private\osgar\osgar\replay.py", line 80, in <module>
    module_instance = replay(args)
  File "c:\admin\_private\osgar\osgar\replay.py", line 61, in replay
    assert application is not None
AssertionError
```
zwn: No. The handling of application-related functionality has been moved from `Recorder` to `config.load`. The code now expects that the driver for the module `app` is the actual module, ie. `osgar.go:Go`. Without this information `osgar.replay` does not know what app is supposed to be. Do you thing such a backward compatibility is really needed?

We _could_ add some cmdline options to `osgar.replay` like `--application=<something>`. I just felt that application logs are such a short lived thing it does not matter (the log replay only works until you change the application code - unlike for example log of camera images).

Oh, I see I forgot some code in `replay` that still works with application, which should not be there anymore so we will need some updates anyway.
m3d: There used to be also option to run recorded data on "different application" - say you use "Go 1 meter" to scan the environment and then try some "explore" because you recorded some interesting corner. I guess that force another configuration may still work??? I will test it on Eduro tonight (35%) and then I would merge it.
zwn: Yes - you can use another configuration. Let me know how you feel about the ergonomics, we can also tweak the details later if required.
m3d: ```
root@voyage:/home/robot/git/osgar# python -m osgar.go run config/eduro-subt.json  --note "test before app config load change" -d 0.2
Namespace(command='run', config=['config/eduro-subt.json'], dist=0.2, note='test before app config load change', speed=None, timeout=10)
Traceback (most recent call last):
  File "/root/miniconda3/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/root/miniconda3/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/robot/git/osgar/osgar/go.py", line 93, in <module>
    config = config_load(args.config, application=Go)
  File "/home/robot/git/osgar/osgar/lib/config.py", line 44, in load
    with open(filename) as f:
TypeError: expected str, bytes or os.PathLike object, not list
```
was it working on your robot?

m3d: I hacked it with
```
root@voyage:/home/robot/git/osgar# git diff
diff --git a/osgar/lib/config.py b/osgar/lib/config.py
index 4f962c6..40e8745 100644
--- a/osgar/lib/config.py
+++ b/osgar/lib/config.py
@@ -41,7 +41,8 @@ def _application2import(application):
 def load(*filenames, application=None):
     ret = {}
     for filename in filenames:
-        with open(filename) as f:
+        print(filename)
+        with open(filename[0]) as f:
             data = json.load(f)
             assert 'version' in data, data
             assert data['version'] in SUPPORTED_VERSIONS, data['version']
```
because the `filename` was `['config/eduro-subt.json']` ... but I am not sure about other consequences.
m3d: Replay then looks like working and in log is stored `{'app': {'driver': 'osgar.go:Go'` or `{'app': {'driver': 'osgar.turn:Turn'`
zwn: Sorry about that bug :worried: . I changed the signature of the `config.load` function and didn't fix all the usages properly afterwards. It is really hard to find and fix all usages of `config.load` due to the fact they are sometimes renamed on import to `config_load` and sometimes not. That is mostly why I renamed it in #256. And why I proposed #255. But it should be fixed now.
m3d: OK, thanks - can you rebase it and prepare for merge? I would test it on Kloubak tonight and if it works then I would merge it, OK?
m3d: maybe merge this after #256 and I will ask for fix in case of failure, OK?
zwn: I have rebased it on current master. I have also fixed `osgar.replay.replay` to the new usage of `config_load`. With this adjustment it allows for backward compatibility using different config file and replacing `application` driver with the supplied driver. However, this isn't directly exposed in anyway (but might be helpful in one-off scripts). Is it ok like this?

Would now be a good time to remove custom replay code from all the "main" modules? It used to be there to allow application to be replayed which can now be handled by the generic replay facility.
zwn: Let me correct myself - it is used in launcher. So when replaying through launcher and custom config file is specified, the supplied application is used.
zwn: Ok, back to the trees as they say :wink: . When using `replay.replay` and supplying custom config file, the application is used. If not supplying custom config, the argument `application` is ignored.

It is not nice, but it is working. I would keep it this way for this PR and refactor later (with the removal of the replay sections in general, on the move to one common launcher).

I am not aware of any further bugs in this PR (:tm:) :wink: 

Patch:
diff --git a/.travis.yml b/.travis.yml
index abba3c2d..02d23815 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -3,8 +3,8 @@ python:
     - "3.6"
     - "3.7"
 script:
-    - python -m unittest discover subt
-    - python -m unittest discover osgar
+    - python -m unittest discover --top-level-directory . --start-directory subt
+    - python -m unittest discover --top-level-directory . --start-directory osgar
 branches:
   only:
     - master
diff --git a/examples/sick2018/sick2018.py b/examples/sick2018/sick2018.py
index b28e5dae..ba92b533 100644
--- a/examples/sick2018/sick2018.py
+++ b/examples/sick2018/sick2018.py
@@ -25,7 +25,7 @@ from osgar.lib.config import config_load
 from osgar.lib.mathex import normalizeAnglePIPI
 from osgar.record import Recorder
 
-from scan_feature import detect_box, detect_transporter
+from .scan_feature import detect_box, detect_transporter
 
 
 # TODO shared place for multiple applications
@@ -290,7 +290,7 @@ if __name__ == "__main__":
     args = parser.parse_args()
 
     if args.command == 'replay':
-        from replay import replay
+        from osgar.replay import replay
         args.module = 'app'
         game = replay(args, application=SICKRobot2018)
         game.verbose = args.verbose
@@ -298,9 +298,9 @@ if __name__ == "__main__":
 
     elif args.command == 'run':
         with LogWriter(prefix='eduro-', note=str(sys.argv)) as log:
-            config = config_load(*args.config)
+            config = config_load(*args.config, application=SICKRobot2018)
             log.write(0, bytes(str(config), 'ascii'))  # write configuration
-            robot = Recorder(config=config['robot'], logger=log, application=SICKRobot2018)
+            robot = Recorder(config=config['robot'], logger=log)
             game = robot.modules['app']  # TODO nicer reference
             robot.start()
             game.play()
diff --git a/osgar/go.py b/osgar/go.py
index 6bd5285e..c9226ad2 100644
--- a/osgar/go.py
+++ b/osgar/go.py
@@ -90,7 +90,7 @@ if __name__ == "__main__":
 
     elif args.command == 'run':
         prefix = 'go-' + os.path.basename(args.config[0]).split('.')[0] + '-'
-        config = config_load(*args.config)
+        config = config_load(*args.config, application=Go)
 
         # apply overrides from command line
         config['robot']['modules']['app']['init']['dist'] = args.dist
@@ -100,7 +100,7 @@ if __name__ == "__main__":
 
         with LogWriter(prefix=prefix, note=str(sys.argv)) as log:
             log.write(0, bytes(str(config), 'ascii'))  # write configuration
-            robot = Recorder(config=config['robot'], logger=log, application=Go)
+            robot = Recorder(config=config['robot'], logger=log)
             game = robot.modules['app']
             robot.start()
             game.join()
diff --git a/osgar/lib/config.py b/osgar/lib/config.py
index a386a7ca..72fe74de 100644
--- a/osgar/lib/config.py
+++ b/osgar/lib/config.py
@@ -2,6 +2,7 @@
   Osgar Config Class
 """
 import json
+import sys
 from importlib import import_module
 
 from osgar.drivers import all_drivers
@@ -26,7 +27,14 @@ class MergeConflictError(Exception):
     pass
 
 
-def config_load(*filenames):
+def _application2import(application):
+    # ModuleSpec: https://www.python.org/dev/peps/pep-0451/
+    # __qualname__: https://www.python.org/dev/peps/pep-3155/
+    s = sys.modules[application.__module__].__spec__
+    return f"{s.name}:{application.__qualname__}"
+
+
+def config_load(*filenames, application=None):
     ret = {}
     for filename in filenames:
         with open(filename) as f:
@@ -34,6 +42,13 @@ def config_load(*filenames):
             assert 'version' in data, data
             assert data['version'] in SUPPORTED_VERSIONS, data['version']
             ret = merge_dict(ret, data)
+    if application is None:
+        return ret
+    if not isinstance(application, str):
+        application = _application2import(application)
+    for module_name, module_config in ret['robot']['modules'].items():
+        if module_config['driver'] == 'application':
+            module_config['driver'] = application
     return ret
 
 
diff --git a/osgar/record.py b/osgar/record.py
index 5fab1b45..32075b10 100644
--- a/osgar/record.py
+++ b/osgar/record.py
@@ -14,7 +14,7 @@ from osgar.bus import BusHandler
 
 
 class Recorder:
-    def __init__(self, config, logger, application=None):
+    def __init__(self, config, logger):
         self.stop_requested = threading.Event()
         self.modules = {}
 
@@ -25,15 +25,8 @@ class Recorder:
                 out[output_type] = []
                 slots[output_type] = []
             bus = BusHandler(logger, out=out, slots=slots, name=module_name)
+            module = get_class_by_name(module_config['driver'])(module_config['init'], bus=bus)
             buses[module_name] = bus
-
-            module_class = module_config['driver']
-            if module_class == 'application':
-                assert application is not None  # external application required
-                module = application(module_config['init'], bus=bus)
-            else:
-                module = get_class_by_name(module_class)(module_config['init'], bus=bus)
-
             self.modules[module_name] = module
 
         for from_module, to_module in config['links']:
@@ -69,14 +62,13 @@ class Recorder:
 
 
 def record(config_filename, log_prefix, duration_sec=None, application=None):
-    if type(config_filename) == str:
-        config = config_load(config_filename)
-    else:
-        config = config_load(*config_filename)
+    if isinstance(config_filename, str):
+        config_filename = [config_filename]
+    config = config_load(*config_filename, application=application)
     with LogWriter(prefix=log_prefix, note=str(sys.argv)) as log:
         try:
             log.write(0, bytes(str(config), 'ascii'))  # write configuration
-            recorder = Recorder(config=config['robot'], logger=log, application=application)
+            recorder = Recorder(config=config['robot'], logger=log)
             recorder.start()
             if application is not None:
                 app = recorder.modules['app']  # TODO nicer reference
diff --git a/osgar/replay.py b/osgar/replay.py
index ee9ab6b1..5d127750 100644
--- a/osgar/replay.py
+++ b/osgar/replay.py
@@ -18,7 +18,7 @@ def replay(args, application=None):
     config_str = next(log)[-1]
     config = literal_eval(config_str.decode('ascii'))
     if args.config is not None:
-        config = config_load(*args.config)
+        config = config_load(*args.config, application=application)
 
     names = logger.lookup_stream_names(args.logfile)
     print("stream names:")
@@ -53,11 +53,7 @@ def replay(args, application=None):
         bus = LogBusHandler(log, inputs=inputs, outputs=outputs)
 
     driver_name = module_config['driver']
-    if driver_name == 'application':
-        assert application is not None
-        module_class = application
-    else:
-        module_class = get_class_by_name(driver_name)
+    module_class = get_class_by_name(driver_name)
     module_instance = module_class(module_config['init'], bus=bus)
     bus.node = module_instance
     return module_instance
diff --git a/osgar/ro2018.py b/osgar/ro2018.py
index a185ffc7..6bf73837 100644
--- a/osgar/ro2018.py
+++ b/osgar/ro2018.py
@@ -227,9 +227,9 @@ if __name__ == "__main__":
 
     elif args.command == 'run':
         with LogWriter(prefix='ro2018-', note=str(sys.argv)) as log:
-            config = config_load(*args.config)
+            config = config_load(*args.config, application=RoboOrienteering2018)
             log.write(0, bytes(str(config), 'ascii'))  # write configuration
-            robot = Recorder(config=config['robot'], logger=log, application=RoboOrienteering2018)
+            robot = Recorder(config=config['robot'], logger=log)
             app = robot.modules['app']  # TODO nicer reference
             robot.start()
             app.play()
diff --git a/osgar/turn.py b/osgar/turn.py
index 7d035497..4f574dc0 100644
--- a/osgar/turn.py
+++ b/osgar/turn.py
@@ -112,7 +112,7 @@ if __name__ == "__main__":
 
     elif args.command == 'run':
         prefix = 'turn-' + os.path.basename(args.config[0]).split('.')[0] + '-'
-        config = config_load(*args.config)
+        config = config_load(*args.config, application=Turn)
 
         # apply overrides from command line
         config['robot']['modules']['app']['init']['angle_deg'] = args.angle
@@ -122,7 +122,7 @@ if __name__ == "__main__":
 
         with LogWriter(prefix=prefix, note=str(sys.argv)) as log:
             log.write(0, bytes(str(config), 'ascii'))  # write configuration
-            robot = Recorder(config=config['robot'], logger=log, application=Turn)
+            robot = Recorder(config=config['robot'], logger=log)
             game = robot.modules['app']
             robot.start()
             game.join()
diff --git a/subt/__init__.py b/subt/__init__.py
new file mode 100644
index 00000000..e69de29b
diff --git a/subt/main.py b/subt/main.py
index 22b762e8..fdf66823 100644
--- a/subt/main.py
+++ b/subt/main.py
@@ -618,7 +618,7 @@ def main():
 
         # support simultaneously multiple platforms
         prefix = os.path.basename(args.config[0]).split('.')[0] + '-'
-        config = config_load(*args.config)
+        config = config_load(*args.config, application=SubTChallenge)
 
         # apply overrides from command line
         config['robot']['modules']['app']['init']['walldist'] = args.walldist
@@ -629,7 +629,7 @@ def main():
 
         with LogWriter(prefix=prefix, note=str(sys.argv)) as log:
             log.write(0, bytes(str(config), 'ascii'))  # write configuration
-            robot = Recorder(config=config['robot'], logger=log, application=SubTChallenge)
+            robot = Recorder(config=config['robot'], logger=log)
             app = robot.modules['app']  # TODO nicer reference
             robot.start()
             app.play()


Test patch:
diff --git a/osgar/lib/test_config.py b/osgar/lib/test_config.py
index 9e86fbfb..18846ee4 100644
--- a/osgar/lib/test_config.py
+++ b/osgar/lib/test_config.py
@@ -54,5 +54,13 @@ class ConfigTest(unittest.TestCase):
         node = get_class_by_name('udp')
         self.assertNotEqual(node, LogTCP)
 
+    def test_application(self):
+        conf_dir = '../../config'
+        filename = test_data('ro2018-spider-gps-imu.json', conf_dir)
+        conf = config_load(filename, application=MyTestRobot)
+        self.assertTrue(conf['robot']['modules']['app']['driver'].endswith('lib.test_config:MyTestRobot'))
+        conf = config_load(filename, application='osgar.lib.test_config:MyTestRobot')
+        self.assertTrue(conf['robot']['modules']['app']['driver'].endswith('lib.test_config:MyTestRobot'))
+
 # vim: expandtab sw=4 ts=4
 
diff --git a/osgar/test_record.py b/osgar/test_record.py
index 57e569e3..284df88e 100644
--- a/osgar/test_record.py
+++ b/osgar/test_record.py
@@ -68,17 +68,5 @@ class RecorderTest(unittest.TestCase):
                     config = json.loads(f.read())
                 recorder = Recorder(config=config['robot'], logger=logger)
 
-    def test_application(self):
-        # plug-in external application
-        with open(os.path.dirname(__file__) + '/../config/ro2018-spider-gps-imu.json') as f:
-            config = json.loads(f.read())
-
-        class DummyApp:
-            def __init__(self, config, bus):
-                self.bus = bus  # needed for linkage
-
-        with patch('osgar.drivers.logserial.serial.Serial') as mock:
-            logger = MagicMock()
-            recorder = Recorder(config=config['robot'], logger=logger, application=DummyApp)
 
 # vim: expandtab sw=4 ts=4