cmdline arguments from config file #525

zbynekwinkler · 2020-05-27T08:40:30Z

There can be a new section robot.arguments in config file. This section contains a dict where key is argument name and value is another dict that is given to argparse add_argument. When a string in the config file equals a name of an argument wrapped in curly brackets (like "{walldist}") it is replaced with the value of the argument given on the command line.

Help is overridden so that it works also for arguments from the config file:

$ python -m osgar.record  subt/zmq-subt-x2.json -h
usage: record.py [-h] [--note NOTE] [--duration DURATION] [--log LOG]
                 [--walldist WALLDIST] --side {left,right,auto}
                 [--speed SPEED] [--timeout TIMEOUT]
                 [--init-offset INIT_OFFSET] [--init-path INIT_PATH]
                 [--start-paused]
                 config [config ...]

Record run given configuration

positional arguments:
  config                configuration file

optional arguments:
  -h, --help            show this help message and exit
  --note NOTE           add description
  --duration DURATION   recording duration (sec), default infinite
  --log LOG             record log filename
  --walldist WALLDIST   distance for wall following (default: 1.0m)
  --side {left,right,auto}
                        which side to follow
  --speed SPEED         maximum speed (1.0m)
  --timeout TIMEOUT     seconds of exploring before going home (default: 600s)
  --init-offset INIT_OFFSET
                        inital 3D offset accepted as a string of comma
                        separated values (meters)
  --init-path INIT_PATH
                        inital path to be followed from (0, 0). 2D coordinates
                        are separated by ;
  --start-paused        start robot Paused and wait for LoRa Contine command

open questions

I was not sure if to put the arguments in config['arguments'] or in config['robot']['arguments'] (now it uses the later). Another question is if it would be better to store the provided arguments and the original config file in the logfile separately or the config with the arguments replaced (now it does the later). Storing them separately would require changes in replay as well so I'd do that as a separate PR.

The main advantage is that a cmdline argument can be used multiple times in the config file and it does not have to be in app.init section. The disadvantage is that the values are strings so they need to be explicitly converted on the application side.

m3d · 2020-05-27T14:03:17Z

I am not sure if I like it ... i.e. I see motivation, but it depends how many things you want to pass as command line and how many as parameter. In virtual it is at the end hardcoded as it called through run_solution.bash ... I do not know, but I prefer number where they have sense (i.e. speed).

zbynekwinkler · 2020-05-27T14:15:09Z

how many things you want to pass as command line and how many as parameter

I don't understand this sentence. For me "command line parameter" is one thing but you seem to see two different things.

zbynekwinkler · 2020-05-27T14:27:44Z

In the long run I'd like to get to a situation where we can just use a generic osgar.record call without the need to create ad-hoc solutions like we currently have in subt.main (with replay we are already there - anything can be replayed by the generic replay facility). So any ideas pushing us closer to that future are welcomed.

zbynekwinkler · 2020-05-27T14:41:31Z

If not having the numbers as numbers but as strings is the only problem, I can add the same logic as is in python json parser. I have just checked and it is really simple (5 lines).

m3d · 2020-05-27T15:27:49Z

In the long run I'd like to get to a situation where we can just use a generic osgar.record call without the need to create ad-hoc solutions like we currently have in subt.main (with replay we are already there - anything can be replayed by the generic replay facility). So any ideas pushing us closer to that future are welcomed.

Thanks for update the motivation - now it makes more sense (which I did not see much in changing subt/main.py. OK

zbynekwinkler · 2020-05-28T14:59:37Z

I bit the bullet and implemented the whole thing - osgar.recod can now load argument definitions from config files.

zbynekwinkler · 2020-05-28T15:23:08Z

Hmm. The tests are not starting at all 😟

zbynekwinkler · 2020-05-29T08:20:54Z

@m3d @jisa It is ready for review.

I was not sure if to put the arguments in config['arguments'] or in config['robot']['arguments'] (now it uses the later). Another question is if it would be better to store the provided arguments and the original config file in the logfile separately or the config with the arguments replaced (now it does the later). Storing them separately would require changes in replay as well so I'd do that as a separate PR.

m3d · 2020-06-01T08:17:47Z

osgar/record.py

@@ -81,16 +83,38 @@ def main():
        format='%(asctime)s %(name)-12s %(levelname)-8s %(message)s',
        datefmt='%Y-%m-%d %H:%M',
    )
-    parser = argparse.ArgumentParser(description='Record run on real HW with given configuration')
-    parser.add_argument('config', nargs='+', help='configuration file')
+    parser = argparse.ArgumentParser(description='Record run given configuration', add_help=False)


why are you overriding default "help"?

I see - you want to allow "unknown arguments" ... OK

Because when I don't, I am not able to load the config file and provide help also for the arguments defined in the config file.

m3d · 2020-06-01T08:27:37Z

in general I like it :). It looks like we may need to switch to some "combined configuration" where we listed more than one config ... i.e. one for application arguments and one for HW, say Kloubak K2. I will have to do some tests to get the "first hand experience". Thanks m.

zbynekwinkler · 2020-06-01T09:20:30Z

Hmm. The tests are not starting at all.

I got response from github support. There reason was that there would have been a merge conflict if a merge would be attempted. In such a case, actions are not run. This is not mentioned in the documentation and also the UI does not provide any hints in such a case. But github is aware of this issue so hopefully, it will improve over time.

m3d · 2020-06-04T09:39:41Z

subt/main.py

-    from osgar.lib.config import config_load
-    from osgar.record import record
-
-    parser = argparse.ArgumentParser(description='SubT Challenge')


Here I see the problem that merge of this change breaks all SubT real robots, I mean they all will have to adapt their JSON configurations.

Hmm, that is right. Not only we would have to update them all, but it would lead to increased duplication 😟. Any ideas?

Well, originally I had in mind extra JSON just with params, but they are closely coupled to the application ... so maybe module app + params ... but then there are the links ... i.e. I do not know yet.

Yeah, back to the drawing board. Making it draft again.

m3d · 2020-06-04T10:31:00Z

There seems to be an error when trying to analyze recorded file via this branch:
python3 -m osgar.logger <logfile>

M:\git\osgar>python -m osgar.logger e:\osgar\logs\subt4-cave\zmq-subt-x2-200604_
092824.log
Traceback (most recent call last):
  File "D:\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 "D:\WinPython-64bit-3.6.2.0Qt5\python-3.6.2.amd64\lib\runpy.py", line 85,
 in _run_code
    exec(code, run_globals)
  File "M:\git\osgar\osgar\logger.py", line 405, in <module>
    main()
  File "M:\git\osgar\osgar\logger.py", line 362, in main
    names, sizes, counts, timestamp = calculate_stat(args.logfile)
  File "M:\git\osgar\osgar\logger.py", line 331, in calculate_stat
    names = ['sys'] + lookup_stream_names(filename)
  File "M:\git\osgar\osgar\logger.py", line 314, in lookup_stream_names
    d = literal_eval(line.decode('ascii'))
  File "D:\WinPython-64bit-3.6.2.0Qt5\python-3.6.2.amd64\lib\ast.py", line 48, i
n literal_eval
    node_or_string = parse(node_or_string, mode='eval')
  File "D:\WinPython-64bit-3.6.2.0Qt5\python-3.6.2.amd64\lib\ast.py", line 35, i
n parse
    return compile(source, filename, mode, PyCF_ONLY_AST)
  File "<unknown>", line 1
    {'version': 2, 'robot': {'arguments': {'walldist': {'help': 'distance for wa
ll following (default: %(default)sm)', 'default': 1.0, 'type': <class 'float'>},
 'side': {'help': 'which side to follow', 'choices': ['left', 'right', 'auto'],
'required': True}, 'speed': {'help': 'maximum speed (%(default)sm)', 'default':
1.0, 'type': <class 'float'>}, 'timeout': {'help': 'seconds of exploring before
going home (default: %(default)ss)', 'default': 600, 'type': <class 'int'>}, 'in
it-offset': {'help': 'inital 3D offset accepted as a string of comma separated v
alues (meters)'}, 'init-path': {'help': 'inital path to be followed from (0, 0).
 2D coordinates are separated by ;'}, 'start-paused': {'dest': 'start_paused', '
action': 'store_true', 'help': 'start robot Paused and wait for LoRa Contine com
mand'}}, 'modules': {'app': {'driver': 'subt.main:SubTChallenge', 'in': ['emerge
ncy_stop', 'pose2d', 'scan', 'rot', 'artf', 'sim_time_sec', 'acc', 'origin'], 'o
ut': ['desired_speed', 'pose2d', 'artf_xyz', 'pose3d', 'stdout', 'request_origin
'], 'init': {'walldist': 0.8, 'side': 'auto', 'speed': 1.0, 'timeout': 100, 'sym
metric': False, 'virtual_bumper_sec': 60, 'virtual_bumper_radius': 10.0, 'virtua
l_world': True}}, 'detector': {'driver': 'subt.artf_node:ArtifactDetectorDNN', '
in': ['image', 'scan'], 'out': ['artf', 'dropped', 'debug_artf', 'stdout'], 'ini
t': {}}, 'reporter': {'driver': 'subt.artifacts:ArtifactReporter', 'in': ['artf_
xyz'], 'out': ['artf_cmd'], 'init': {}}, 'transmitter': {'driver': 'zeromq', 'in
': [], 'out': ['raw'], 'init': {'mode': 'PUSH', 'endpoint': 'tcp://localhost:555
6'}}, 'receiver': {'driver': 'zeromq', 'in': [], 'out': ['raw'], 'init': {'mode'
: 'PULL', 'endpoint': 'tcp://localhost:5555'}}, 'rosmsg': {'driver': 'rosmsg', '
in': ['slot_raw', 'desired_speed', 'tick', 'stdout', 'request_origin'], 'out': [
'rot', 'acc', 'scan', 'image', 'pose2d', 'sim_time_sec', 'cmd', 'origin', 'gas_d
etected', 'orientation'], 'init': {'downsample': 2}}, 'depth2scan': {'driver': '
subt.depth2scan:DepthToScan', 'in': ['depth', 'scan'], 'out': ['scan'], 'init':
{}}}, 'links': [['app.desired_speed', 'rosmsg.desired_speed'], ['app.stdout', 'r
osmsg.stdout'], ['app.request_origin', 'rosmsg.request_origin'], ['rosmsg.origin
', 'app.origin'], ['receiver.raw', 'rosmsg.raw'], ['rosmsg.cmd', 'transmitter.ra
w'], ['rosmsg.rot', 'app.rot'], ['rosmsg.rot', 'depth2scan.rot'], ['rosmsg.acc',
 'app.acc'], ['rosmsg.orientation', 'app.orientation'], ['rosmsg.scan', 'detecto
r.scan'], ['rosmsg.scan', 'depth2scan.scan'], ['rosmsg.image', 'detector.image']
, ['rosmsg.image', 'app.image'], ['rosmsg.depth', 'depth2scan.depth'], ['depth2s
can.scan', 'app.scan'], ['rosmsg.depth', 'detector.depth'], ['rosmsg.pose2d', 'a
pp.pose2d'], ['rosmsg.sim_time_sec', 'app.sim_time_sec'], ['rosmsg.gas_detected'
, 'detector.gas_detected'], ['detector.artf', 'app.artf'], ['detector.stdout', '
rosmsg.stdout'], ['app.artf_xyz', 'reporter.artf_xyz'], ['reporter.artf_cmd', 't
ransmitter.raw']]}}

                                                               ^
SyntaxError: invalid syntax

m3d · 2020-06-04T10:40:27Z

The same problem if you would like to use lidarview on recorded file

zbynekwinkler · 2020-06-04T11:34:20Z

The problem with invalid syntax seems to be related to 'type': <class 'float'> values. But since the whole thing needs to be re-thought, I won't worry about it to much for now. Thanks for testing and feedback 👍 .

m3d · 2020-06-04T11:43:42Z

OK, no problem ... & thanks for the float hint

zbynekwinkler requested review from jisa and m3d May 27, 2020 08:40

This was referenced May 27, 2020

subt: add rospy+zmq based communication with cloudsim #523

Merged

subt.launch: add generic process launcher #511

Merged

Implement generic argument machinery for osgar.record.

a527ad8

zbynekwinkler added 2 commits May 28, 2020 17:02

subt.main: some further simplifications

2065cf3

osgar.record: allow setting of log filename

842b8a5

zbynekwinkler added 3 commits May 29, 2020 09:42

Merge branch 'master' into feature/config-params

f2a990a

subt.main: provide backwards compatibility for max_speed setting

d3350cf

subt.main: either speed or max_speed key is required

5daa58e

m3d reviewed Jun 1, 2020

View reviewed changes

zbynekwinkler changed the title ~~Allow templated variables in config files.~~ cmdline arguments from config file Jun 2, 2020

m3d reviewed Jun 4, 2020

View reviewed changes

m3d requested a review from tajgr June 4, 2020 09:41

zbynekwinkler marked this pull request as draft June 4, 2020 11:34

zbynekwinkler mentioned this pull request Sep 8, 2020

osgar.lib.config: allow overrides in dict merging #612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmdline arguments from config file #525

cmdline arguments from config file #525

zbynekwinkler commented May 27, 2020 •

edited

Loading

m3d commented May 27, 2020

zbynekwinkler commented May 27, 2020

zbynekwinkler commented May 27, 2020

zbynekwinkler commented May 27, 2020

m3d commented May 27, 2020

zbynekwinkler commented May 28, 2020

zbynekwinkler commented May 28, 2020

zbynekwinkler commented May 29, 2020

m3d Jun 1, 2020

m3d Jun 1, 2020

zbynekwinkler Jun 1, 2020

m3d commented Jun 1, 2020

zbynekwinkler commented Jun 1, 2020

m3d Jun 4, 2020

zbynekwinkler Jun 4, 2020

m3d Jun 4, 2020

zbynekwinkler Jun 4, 2020

m3d commented Jun 4, 2020 •

edited

Loading

m3d commented Jun 4, 2020

zbynekwinkler commented Jun 4, 2020

m3d commented Jun 4, 2020

cmdline arguments from config file #525

Are you sure you want to change the base?

cmdline arguments from config file #525

Conversation

zbynekwinkler commented May 27, 2020 • edited Loading

m3d commented May 27, 2020

zbynekwinkler commented May 27, 2020

zbynekwinkler commented May 27, 2020

zbynekwinkler commented May 27, 2020

m3d commented May 27, 2020

zbynekwinkler commented May 28, 2020

zbynekwinkler commented May 28, 2020

zbynekwinkler commented May 29, 2020

m3d Jun 1, 2020

Choose a reason for hiding this comment

m3d Jun 1, 2020

Choose a reason for hiding this comment

zbynekwinkler Jun 1, 2020

Choose a reason for hiding this comment

m3d commented Jun 1, 2020

zbynekwinkler commented Jun 1, 2020

m3d Jun 4, 2020

Choose a reason for hiding this comment

zbynekwinkler Jun 4, 2020

Choose a reason for hiding this comment

m3d Jun 4, 2020

Choose a reason for hiding this comment

zbynekwinkler Jun 4, 2020

Choose a reason for hiding this comment

m3d commented Jun 4, 2020 • edited Loading

m3d commented Jun 4, 2020

zbynekwinkler commented Jun 4, 2020

m3d commented Jun 4, 2020

zbynekwinkler commented May 27, 2020 •

edited

Loading

m3d commented Jun 4, 2020 •

edited

Loading