Project

General

Profile

Feature #461

Passing of arguments (e.g. configuration, datacards)

Added by Rogers, Chris over 10 years ago. Updated almost 10 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
common_py
Target version:
Start date:
23 May 2011
Due date:
12 August 2011
% Done:

100%

Estimated time:
Workflow:

Description

Propose use of Python argparse module to enable passing of arguments to Go:

-d --datacards filename for old G4MICE style datacards
-c --configuration filename for json document containing configuration datacards

Requires it's own module probably

#1

Updated by Tunnell, Christopher over 10 years ago

This would be a good self-contained piece of work to pass off to somebody... want to detach a name from it and see if anybody else wants it? It's just everything has your name on it :)

#2

Updated by Rogers, Chris over 10 years ago

  • Assignee changed from Rogers, Chris to Robinson, Matthew

Matt, would this be something that you'd be interested in doing?

#3

Updated by Rogers, Chris over 10 years ago

  • Assignee changed from Robinson, Matthew to Heidt, Christopher

I asked Chris H to have a look at this as there's been no motion on it for a few weeks - hope that's okay...

#4

Updated by Tunnell, Christopher about 10 years ago

Chris Heidt should post a demo here. We can hammer out the specification either after the demo or once we do the face to face.

#5

Updated by Rogers, Chris about 10 years ago

How's this one going?

#6

Updated by Rogers, Chris about 10 years ago

  • Due date set to 02 August 2011

Hi Chris, I thought it might help progress to put a deadline on the code... say two weeks from now.

#7

Updated by Heidt, Christopher about 10 years ago

  • File Configuration.py added

I have committed and pushed the change to Configuration.py to launchpad. There was some talk about testing as well, what more do I need to do?

#8

Updated by Rogers, Chris about 10 years ago

  • Assignee changed from Heidt, Christopher to Rogers, Chris

Great I'll have a look...

#9

Updated by Rogers, Chris about 10 years ago

  • Assignee changed from Rogers, Chris to Heidt, Christopher

This looks good and runs okay. Comments:

BUG I noticed is that your indentation was wrong, so I'm not quite sure whether the execution of the argparse code is done when the class is defined or when the function is called. You should set up your text editor to indent with spaces and not tabs.

DOC You need to update the function documentation for getConfigJson. Also this should be a docstring (not your fault I realise, but...)

Could you maybe write some tests also?

Testing for this could be quite interesting. You can potentially mock passing command line arguments by writing fake values to sys.argv - if that doesn't work then you will have to make a small script that basically checks that the configuration is set up with appropriate parameters. Then try passing different parameters to the argparse and see what happens.
  • No parameters, check ConfigurationDefaults are set up correctly
  • Bad parameters, check Configuration throws an exception
  • Good parameters, check Configuration returns with new parameters and defaults.

Also, try running pylint over Configuration.py and fix any errors or warnings. Note that this may mean fixing some existing problems (that you didn't introduce). Run pylint by simply doing

pylint Configuration.py

Bounce back to Heidt

#10

Updated by Rogers, Chris about 10 years ago

How's progress on this? Deadline was today...

#11

Updated by Heidt, Christopher about 10 years ago

I've made the corrections that you asked, so that much is done, I think. However, I've been trying to figure out how to write a unit test for something that takes command line arguments and I've been coming up blank. I tried playing with sys.argv as you recommended, but I wasn't able to write to it. You also suggested writing a script, but I do not know of a way to write a script to execute a command line, so I've run into a wall there as well. Doing a google search on unit testing argparse only turns up a trick someone used to get around the need for a command line input, rather than a proper unit test for the argparse function.

Anyway I'll upload my work tonight (in a few minutes) if you could take a look and comment on the formatting and content of the documentation I wrote, then I can get that cleaned up rather quickly.

#12

Updated by Rogers, Chris about 10 years ago

You should have asked.

Can call a script using either

os.system

or better http://docs.python.org/library/subprocess.html
subprocess.popen

#13

Updated by Tunnell, Christopher about 10 years ago

If you set sys.argv within the test, then call Configuration() it should see it.

import sys
sys.argv=['application_name', '--do-something']

#14

Updated by Heidt, Christopher about 10 years ago

  • File out.txt added
  • File Configuration.py added

I was able to get the outlines of a test working today, however I ran into a problem that I hope someone might have some insight to. In writing the test I used the sys.argv method to change sigmaPz = 5 to sigmaPz = 15, however when doing the assert test I got an error, 15 != '15'. This means argparse is taking the data and writing it in as a string rather than an integer.

I tried getting around this by overriding the type and forcing the numerical variables to be float, while simultaneously using an exception handler to catch things that should be strings and leave them be. However, this caused a problem in Go.py for some reason:

File "/home/chris/maushole/working_maus/src/common_py/Go.py", line 104, in NativePythonMapReduce
assert(self.mapper.birth(self.jsonConfigDocument) == True)

I'm going to include a couple files with this to hopefully give a more complete idea of my problem.

I'm wondering now if it wouldn't be easier to try to do something clever by reading directly from sys.argv rather than use argparse.

#15

Updated by Rogers, Chris about 10 years ago

Well, any stuff taken from command line will always be a string, so argparse is doing the right thing.

How do you handle the case where we want something with string type, but numerical value. Like what if we want something to have string value '15'? Suggest taking type from ConfigurationDefaults and try casting to this type... leave the exception if this fails, it means bad user input.

#16

Updated by Tunnell, Christopher about 10 years ago

15 int('15')

15.0 float('15')

FYI

Similarly: str(15.0) == '15'

And I'd strongly recommend using argparse instead of reinventing the wheel

#17

Updated by Heidt, Christopher about 10 years ago

I checked the type associated with the ConfigurationDefaults and was able to fix the problem, though it only checks against the types we are currently using. Would it be possible to do something like:

if type(Default) != type(New):
type(New) = type(Default)

#18

Updated by Heidt, Christopher about 10 years ago

Also I don't think there is a way to change simulation_reference_particle's (which is a dict in itself) default value using this method.

#19

Updated by Rogers, Chris about 10 years ago

That's fine - we can change primitive types on command line, changing anything more complex we can do as an upgrade.

#20

Updated by Heidt, Christopher about 10 years ago

I've pushed my revision to Configuration.py to my branch, as it stands right now it is good for entering int, float, str, and bool types. I was working on some recursion stuff to try and get at the dict type variable, but I think that will take some time and I wanted to get this up.

I've also added the ability to specify a file with the -card command, I know this isn't what you were looking for, but I figured it didn't hurt to throw that in as well. Besides using a datacard will give you the ability to work with dict types.

Revision 587 is the Configuration.py file
Revision 588 is the tests I wrote

#21

Updated by Tunnell, Christopher about 10 years ago

  • Due date changed from 02 August 2011 to 12 August 2011
  • Assignee changed from Heidt, Christopher to Tunnell, Christopher

It's looking nice. Small comment till I get time to peak at the tests:

consider this for features that don't exist:

raise NotImplementedError("hi")

and it may be easier to test if you pull your logic out into a function (but haven't looked at tests yet). Will do this week.

#22

Updated by Rogers, Chris about 10 years ago

But just to be explicit - please put a sensible error message in, even if it doesnt say anything (like "Function not implemented")

#23

Updated by Tunnell, Christopher about 10 years ago

That's what goes in instead of 'hi'

#24

Updated by Heidt, Christopher about 10 years ago

ok, I've updated the code so that it prints out an error if the variable simulation_reference_particle differs from that stored in the Default Configuration file, which is currently our only dict type variable in the file. Error message reads:

Unable to modify variable simulation_reference_particle
The feature has not yet been implemented into code.

Then exits the back to the command line using sys.exit().

#25

Updated by Rogers, Chris about 10 years ago

Should raise an exception. Exceptions can be handled if we want - or return to command line if we want also.

sys.exit() is a very aggressive way to deal with bad input.

#26

Updated by Tunnell, Christopher about 10 years ago

Change sys.exit to raise NotImplementedError("bad input") and see what happens. It should exit and say why.

#27

Updated by Tunnell, Christopher about 10 years ago

This should take priority over tracker code review (the tracker code is rejected till it compiles).

I'm looking here:

http://bazaar.launchpad.net/~cheid001/maus/devel/view/head:/src/common_py/Configuration.py

Delete 54-56 since it's too specific and this code is meant to be very general. The way of addressing that situation is by changing like 66 to:

raise NotImplementedError("Unable to parse complex types. See issue #461.")

You need to update lines 40 and 29-38 to explain what the function does with your changes. It would be nice to add some comments within the code to explain (to new people to the code) what you're trying to do with parsing the arguments. This will appear here:

http://micewww.pp.rl.ac.uk/embedded/maus/doxygen_framework/html/classcommon__py_1_1Configuration_1_1Configuration.html

when it's commited.

Can you run:

bzr merge lp:maus

to make sure you're in sync with the trunk? Makes it easier for us to include the fixes and make sure it doesn't break anything.

This is really cool! I'm running it and it's very useful! Nice!

When you run bin/simulate_mice.py do you get a weird error?

#28

Updated by Tunnell, Christopher about 10 years ago

  • Assignee changed from Tunnell, Christopher to Heidt, Christopher
#29

Updated by Tunnell, Christopher about 10 years ago

  • Status changed from Open to In Progress
  • % Done changed from 0 to 50
#30

Updated by Heidt, Christopher about 10 years ago

Tunnell - I do all of my code testing with ./simulate_mice, I get no errors, weird or otherwise when I run.

I'm pretty sure I'm all merged up as of right after my previous commit, I think this version of Configuration.py should incorporate everything you guys have done.

I caught an error with changing the boolean stuff which I have fixed. I changed how errors are handled for dict type arguments so that they would behave how you specified, in addition to that I've also written a lot of new exception handlers. The new changes will recognize when you do something silly like change sigmaPz = 5 to sigmaPz = bob and raise an exception, however, the program will no longer crash if you change sigmaPz to 5.25, it will simply change it from an int type to a float.

Finally, I added a lot of commenting everywhere (except for the new stuff at the end which I didn't look at).

#31

Updated by Rogers, Chris about 10 years ago

  • Target version changed from Future MAUS release to MAUS-v0.1.0
#32

Updated by Heidt, Christopher about 10 years ago

  • File deleted (out.txt)
#33

Updated by Heidt, Christopher about 10 years ago

  • File deleted (Configuration.py)
#34

Updated by Heidt, Christopher about 10 years ago

  • File deleted (Configuration.py)
#35

Updated by Rogers, Chris about 10 years ago

  • Assignee changed from Heidt, Christopher to Rogers, Chris

Awaiting code review

#36

Updated by Rogers, Chris about 10 years ago

Where did you put the regression tests? I couldn't see anything in tests/py_unit/test_core_configuration.py

Rogers edit: fix the file name to be meaningful

#37

Updated by Rogers, Chris about 10 years ago

I wanted to get this into MAUS finally, so:
  • pulled this into Rogers branch
  • moved the command line stuff into a subfunction
  • Added protection to stop command line argument parsing unless explicitly requested. This stops the whole thing breaking when it tries to parse the command line arguments from e.g. nosetests
  • added regression tests
  • fixed all so they pass pylint

Please see:
http://bazaar.launchpad.net/~chris-rogers/maus/devel/view/head:/src/common_py/Configuration.py
http://bazaar.launchpad.net/~chris-rogers/maus/devel/view/head:/tests/py_unit/test_configuration.py

#38

Updated by Rogers, Chris almost 10 years ago

  • Status changed from In Progress to Closed
  • % Done changed from 50 to 100

Committed in r634

#39

Updated by Rogers, Chris almost 10 years ago

  • Target version changed from MAUS-v0.1.0 to MAUS-v0.0.7

Also available in: Atom PDF