Project

General

Profile

Feature #723

Command line arguements cleanup

Added by Tunnell, Christopher almost 10 years ago. Updated over 9 years ago.

Status:
Closed
Priority:
Normal
Category:
common_py
Target version:
Start date:
03 October 2011
Due date:
27 October 2011
% Done:

100%

Estimated time:
Workflow:
Closed

Description

Sort keys so they are listed in alphabetical order

Include --help, --usage without python grabbing it.

Why are long strings starting with '-' instead of the standard '--'?

#1

Updated by Tunnell, Christopher almost 10 years ago

-h doesn't work

#2

Updated by Tunnell, Christopher almost 10 years ago

you probably dont' want to be able to set maus version number...

#3

Updated by Rogers, Chris almost 10 years ago

  • Target version set to Future MAUS release

Good points. But one thing, setting the version number will result in an error. It's a bit untidy though.

#4

Updated by Heidt, Christopher almost 10 years ago

I know I wasn't consulted on this issue, but I saw it there and thought I could get this done real quick. I've pushed an updated version of Configuration.py that changes '-' to '--', it also alphabetized the keys when they are shown, and I have removed -h and --help from the list parsed keys (I'm not sure you really want the help keys gone, but I figured I would do it, and if you don't want it I can always change it back.). So it is up if either of you want to look at it.

#5

Updated by Tunnell, Christopher almost 10 years ago

  • Assignee changed from Rogers, Chris to Heidt, Christopher

I peaked at what it would mean to merge you branch:

lp:~cheid001/maus/devel

with the trunk and there was an enormous amount of changes. I don't know how long it would take but can you use a fresh branch of the trunk to start with and then do something like:

bzr push lp:~cheid001/maus/devel-command-arg-tweaks

I doubt Chris will get to this this week and it's not many changes so shouldn't take long for you. But just to be clear: both --usage and --help should display the info that's displayed when something bad happens (ie. how to use the command line arguments).

#6

Updated by Heidt, Christopher almost 10 years ago

That is incredibly frustrating, I thought I spent this morning updating my branch to the trunk, if that didn't happen I'm kind of at a loss on what to do. I'll try and find some time tonight to figure out what I did wrong...

as for the code I submitted, I did not check --usage, but --help will give you the results you mentioned.

#7

Updated by Tunnell, Christopher almost 10 years ago

Well try it yourself; the following is a nice crosscheck

bzr co lp:maus maus_some_name_blah
cd maus_some_name_blah
bzr merge --preview lp:~cheid001/maus/devel

And you should see nothing if there are no differences. Or minor changes if there are minor changes.

I think Chris added some stuff to what you did so that may explain the differences. Code evolves: I spent ages making MapCppSimulation look the way I wanted then others came in and improved it beyond my recognition.

#8

Updated by Rogers, Chris almost 10 years ago

Tunnell, Christopher wrote:

Code evolves: I spent ages making MapCppSimulation look the way I wanted then others came in and improved it beyond my recognition.

Sorry about that...

#9

Updated by Heidt, Christopher almost 10 years ago

I'm fairly confident that everything on my end is updated now...

I see the problem now with the -h or --help arguments. Doing some research has lead me to believe the problem arises due to ROOT grabbing the arguments (http://root.cern.ch/phpBB3/viewtopic.php?f=14&t=4762). However, I'm not sure what a proper solution to this problem is, yet.

#10

Updated by Tunnell, Christopher almost 10 years ago

Will look at the branch tomorrow.

The way to get around the ROOT issue is by doing something like:

my_sysargv = sys.argv
sys.argv = []
import ROOT
sys.argv = my_sysargv

or something like that. I can explain more or find an example of it I wrote years ago.

#11

Updated by Rogers, Chris almost 10 years ago

To do it this way - will have to happen the first time we import ROOT. So could put it in MAUS.py - but note this is built dynamically by SConstruct (ack!). Better would be to use argparse. I think, for example, it would be better to use usage thing:

http://docs.python.org/dev/library/argparse.html#usage

#12

Updated by Tunnell, Christopher almost 10 years ago

Of course it's build dynamically. The modules it includes are dynamic...

#13

Updated by Rogers, Chris almost 10 years ago

Sorry, didn't mean to criticise. It's correct behaviour, just means messing with this stuff is more difficult.

But we need to put the import ROOT wrapper somewhere, probably it has to go in here, we need to then tell everyone to "always import MAUS first" for this patch which is just a bit dirty. Better to try to go through argparse directly I would say.

#14

Updated by Heidt, Christopher almost 10 years ago

I browsed the Sconstruct file and was able to identify how MAUS.py is created, however, currently there is nothing about ROOT in there (which I'm sure you both already know...). Where in the code is ROOT imported?

I don't follow how we would use the usage. It seems like since the problem is ROOT picking up the command before argparse can get to it, that no matter what we do to argparse, it will have no effect on ROOT grabbing these terms.

#15

Updated by Rogers, Chris almost 10 years ago

Aha!

http://root.cern.ch/phpBB3/viewtopic.php?f=14&t=11314

ROOT.PyConfig.IgnoreCommandLineOptions = True

#16

Updated by Tunnell, Christopher almost 10 years ago

Chris: can you have MAUS.py import ROOT as the first thing? The SConstruct file should be just like python. It sounds like you already found where it creates MAUS.py.

Just add:

"import ROOT"

at the top. I'm not convinced Chris's link will work because I think it tries to parse the command line arguments when it imports ROOT.

Can you (using a branch without many differences to the trunk to make it easier for us to merge it in) add that? And the:

my_sys.argv = sys.argv
sys.argv = sys.argv[0:1]
import ROOT
sys.argv = my_sys.argv

and see if that works? And the other changes you/we mentioned?

#17

Updated by Tunnell, Christopher almost 10 years ago

Ack: hash collision. Previous message at Heidt referencing Roger's idea.

#18

Updated by Rogers, Chris almost 10 years ago

It's a weird thing - try the script here:

#!/usr/bin/env python

import ROOT
ROOT.PyConfig.IgnoreCommandLineOptions = True

c = ROOT.TCanvas()

As stated, setting IgnoreCommandLineOptions flag does have the desired effect. ROOT only parses the command line options when the TCanvas is created (comment out this line and ROOT doesn't parse command line options at all). Probably why I never noticed the bug in testing... so probably either code will have the same effect, and the condition is that we have to call

import MAUS

before we instantiate any ROOT objects. I prefer the "official" method using IgnoreCommandLineOptions as it is less likely to break in the future (i.e. ROOT change the way they call system arguments or whatever, which breaks our hack but not their "official" method). But best probably to just put in an appropriate regression test and then whatever happens we will catch it...

All a bit dirty and hacky.

#19

Updated by Rogers, Chris almost 10 years ago

Should just say - doesn't really matter what you do though, just something reasonable and make a test for it...

#20

Updated by Heidt, Christopher almost 10 years ago

I was having some problems Tunnell's recommendation at the end of last week. Implemented Rogers' suggestion, seems to be working. In the process of committing and pushing revisions now.

#21

Updated by Tunnell, Christopher almost 10 years ago

The last time I did my suggestion was years ago, so maybe there are issues. If Roger's suggestion works, great.

I never knew that it only parsed the arguments when TCanvas was created. I assumed it happened at import ROOT. Given that, Chris Roger's suggestion seems best. Let us know when the code is ready.

#22

Updated by Rogers, Chris almost 10 years ago

  • Due date set to 27 October 2011

What's the status here? I think aim for completion by collaboration meeting?

#23

Updated by Tunnell, Christopher almost 10 years ago

Or sooner if possible so people can play with it at the workshop?

#24

Updated by Heidt, Christopher almost 10 years ago

The first draft has been done for about 3 days, I believe it has been pushed to my branch on launchpad. I have been running some test over the last few days to make sure everything works with the new code and this has taken a lot of time (I need to download a fresh branch and re-install everything each time I change something).

Something is breaking with the new code, I think the unit test at the end of install_then_build_then_test.bash are coming up against unexpected results.

It would be very helpful if someone could remind me how to reroute error messages to a log file from the command line. I tried ./test/integration/install_then_build_then_test.bash 2> errorlog.txt, but this seems to have rerouted all messages... is it '1>'?

#25

Updated by Tunnell, Christopher almost 10 years ago

Per last thing about rerouting (no space I guess) look here: http://micewww.pp.rl.ac.uk/issues/681#note-6

You can do:

bzr merge lp:maus

to get a copy of the latest code instead of checking out a new version. Checking out a new version is really a last resort and unless something like the geant4 version changes, it shouldn't be necessary.

So the code we find here:

https://code.launchpad.net/maus

under your name is the code with these changes?

#27

Updated by Rogers, Chris almost 10 years ago

The first draft has been done for about 3 days, I believe it has been pushed to my branch on launchpad. I have been running some test over the last few days to make
sure everything works with the new code and this has taken a lot of time (I need to download a fresh branch and re-install everything each time I change something).

Why do you need to download a fresh branch? Really should try to avoid this, because correct it is very time consuming.

Anyway, let me know when you are ready to merge.

#28

Updated by Rogers, Chris almost 10 years ago

You also need to fix the regression tests - these check the code works. Currently in your branch I think

nosetests tests/py_unit/test_configuration.py

returns whole load of errors like


======================================================================
ERROR: Test parsing string from command line to configuration
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/cr67/G4MICE/MAUS/maus_trunk/third_party/install/lib/python2.7/site-packages/nose-1.0.0-py2.7.egg/nose/case.py", line 133, in run
    self.runTest(result)
  File "/home/cr67/G4MICE/MAUS/maus_trunk/third_party/install/lib/python2.7/site-packages/nose-1.0.0-py2.7.egg/nose/case.py", line 151, in runTest
    test(result)
  File "/home/cr67/G4MICE/MAUS/maus_trunk/third_party/install/lib/python2.7/unittest/case.py", line 376, in __call__
    return self.run(*args, **kwds)
  File "/home/cr67/G4MICE/MAUS/maus_trunk/third_party/install/lib/python2.7/unittest/case.py", line 318, in run
    testMethod()
  File "/home/cr67/G4MICE/MAUS/maus_trunk/tests/py_unit/test_configuration.py", line 101, in test_command_line_args_str
    "input_string":""})
  File "/home/cr67/G4MICE/MAUS/maus_trunk/src/common_py/Configuration.py", line 98, in command_line_arguments
    results = parser.parse_args()
  File "/home/cr67/G4MICE/MAUS/maus_trunk/third_party/install/lib/python2.7/argparse.py", line 1659, in parse_args
    self.error(msg % ' '.join(argv))
  File "/home/cr67/G4MICE/MAUS/maus_trunk/third_party/install/lib/python2.7/argparse.py", line 2311, in error
    self.exit(2, _('%s: error: %s\n') % (self.prog, message))
  File "/home/cr67/G4MICE/MAUS/maus_trunk/third_party/install/lib/python2.7/argparse.py", line 2299, in exit
    _sys.exit(status)
SystemExit: 2

----------------------------------------------------------------------

So you need to

  • Check that current tests work okay (currently they do not in your branch)
  • Add an extra test that imports ROOT and check that we can still parse arguments correctly.

Have a look at Unit_tests

#29

Updated by Heidt, Christopher almost 10 years ago

The bugs have been fixed, everything looks pretty good. Still need to write a test for importing ROOT, I am having some trouble with the test, how can I test if the correct help menu is being displayed?

#30

Updated by Rogers, Chris almost 10 years ago

I think you have to do something like call simulate_mice.py using subprocess module.

There are ways to catch system exit calls (SystemExit exception), and to catch standard output (sys.stdout, sys.stderr) - but PyRoot looks like it is not calling the usual python system exit mechanism, nor using the usual output streams. So that doesn't work.

So something like

test_out = open(os.environ['MAUS_ROOT_DIR']+'tmp/no_root_help_test'], 'w')
subprocess.Popen(['simulate_mice.py', '--help'], stdout=test_out, stderr=test_out)
test_out.close()
test_in = open(os.environ['MAUS_ROOT_DIR']+'tmp/no_root_help_test'])
first_line = test_in.readline()

Then do some appropriate checks on the first_line. Might be worth adding something to the argparse help anyway, and that would make a useful thing to look for.

#31

Updated by Rogers, Chris almost 10 years ago

I should say - normally we don't like to test things like this, because our test now depends on simulate_mice.py not changing. But this is such a lurking evil that if we don't check for it, we will surely trip up on it again.

#32

Updated by Heidt, Christopher almost 10 years ago

I'm submitted two branches to be merged into the code for this issue. I'm afraid that the first submission is so mucked up from bzr issues that it will not properly merge into the trunk.

#33

Updated by Rogers, Chris over 9 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100
  • Workflow set to Closed

Merged in r629.

#34

Updated by Rogers, Chris over 9 years ago

  • Target version changed from Future MAUS release to MAUS-v0.1.2

Also available in: Atom PDF