-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmdline arguments from config file #525
base: master
Are you sure you want to change the base?
Conversation
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.
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 |
I don't understand this sentence. For me "command line parameter" is one thing but you seem to see two different things. |
In the long run I'd like to get to a situation where we can just use a generic |
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). |
Thanks for update the motivation - now it makes more sense (which I did not see much in changing |
I bit the bullet and implemented the whole thing - |
Hmm. The tests are not starting at all 😟 |
@m3d @jisa It is ready for review. I was not sure if to put the arguments in |
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you overriding default "help"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - you want to allow "unknown arguments" ... OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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. |
from osgar.lib.config import config_load | ||
from osgar.record import record | ||
|
||
parser = argparse.ArgumentParser(description='SubT Challenge') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that is right. Not only we would have to update them all, but it would lead to increased duplication 😟. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, back to the drawing board. Making it draft again.
There seems to be an error when trying to analyze recorded file via this branch:
|
The same problem if you would like to use |
The problem with invalid syntax seems to be related to |
OK, no problem ... & thanks for the |
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 argparseadd_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:
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.