Modify

Opened 8 years ago

Last modified 6 years ago

#221 reopened defect

Simfactory should complain about unused arguments

Reported by: Erik Schnetter Owned by: mthomas
Priority: major Milestone:
Component: SimFactory Version:
Keywords: Cc:

Description

When more arguments are given to simfactory that it uses, it should complain with an error message.

For example, "sim stop sim1 sim2" stops simulation sim1 and ignores the argument sim2. Instead, it should output an error message if the argument sim2 is ignored.

Attachments (1)

unused_option.patch (1.5 KB) - added by Roland Haas 6 years ago.
complain about unused options a la PETSc

Download all attachments as: .zip

Change History (9)

comment:1 Changed 7 years ago by Ian Hinder

Each command should also complain about any options that it doesn't take.

Changed 6 years ago by Roland Haas

Attachment: unused_option.patch added

complain about unused options a la PETSc

comment:2 Changed 6 years ago by Roland Haas

PETSc has the same problem: a central options database of all command line options but no central place where options are defined so the main code does not know which options are valid. Their solution is to warn if at the end of the run, there are any never used options left in the database. The attached patch implements this feature for command line options (but does nothing for eg options taken from machine databases, since those are always present). This is a band-aid until we can come up with something better. It also does nothing about unused arguments, since there does not seem to be a central location where access to arguments could be tracked.

comment:3 Changed 6 years ago by Erik Schnetter

Does it work? Or do some routines access options bypassing the mechanism you implement?

comment:4 Changed 6 years ago by Erik Schnetter

Status: newreview

comment:5 Changed 6 years ago by Roland Haas

Well. Yes and no. It works as well as the PETSc equivalent, which I always found to be lacking, in that it outputs the error after the fact.
Eg.

sim create --queue development foo --configuration sim --parfile foo.bar --procs 156 --num-threads 6 --walltime 0:10:00

will result in simfactory creating the simulation and then complaining

Error: Command line option(s) '--procs,--walltime,--queue,--num-threads' were not used. Most likely 'create' does not support them.

On the other hand doing something like sim show-output --follow foo after foo has finished will complain about --follow not being used since simfactory only considers this option while a simulation is running. The SpEC (which uses the same system) way of dealing with this is to request these unused options anyway to pacify the OptionsParser. So maybe a warning woud be more appropriate though warnings seem to be only output when run with --verbose, yes?
There are ways to get to options that would not be caught (access to the raw option string is possible) and while I would expect that no well behaved routine does that, I have no idea of how simfactory behaves internally.

comment:6 Changed 6 years ago by anonymous

Status: reviewreopened

Ok, this sounds like the current patch doesn't really work and needs improvements / another approach. Could you take a look at using the SpEC-approach within simfactory Roland?

comment:7 Changed 6 years ago by Ian Hinder

Regarding arguments:

Why don't we just add code to each command to check that there are no unused arguments after processing the command line? There are not so many simfactory commands. Also, some of them take a variable number of arguments, so there is no way to determine which ones should be provided except within the commands themselves.

Regarding options:

Is the problem that we don't distinguish between global options and command options? If we could have an option database for the "sim" command, and then one for each of the subcommands, it would be easy to sort out the mess. The position of the option relative to the command name in the sim command line would indicate whether it was a global option or a command option. In the case of a command option, if it wasn't appropriate for that command, it wouldn't be in the command option database, and the option framework would complain about it. SimFactory uses the Python "optparse" module (http://docs.python.org/library/optparse.html). The optparse.parse_args() function usually reads options from the command line no matter where they are, and this prevents us from distinguishing between options before and after the command. However, according to http://docs.python.org/library/optparse.html#querying-and-manipulating-your-option-parser, you can use the OptionParser.disable_interspersed_args() feature to force optparse to stop parsing options as soon as it finds a positional argument. optparse will then return all the remaining arguments in a list, which can then be used with a new optionparser, depending on the command which is used.

comment:8 Changed 6 years ago by Erik Schnetter

Simfactory contains already code to distinguish between global options (valid for all commands) and "local" options (valid only for certain subsystems). Unfortunately, this distinction turns out to be inconvenient; for example, --parfile is defined in the "simulation management" subsystem, but this means it is valid for both "create" (good) and "submit" (unused). We should modify this scheme to have options not for subsystems, but for individual commands instead.

At the same time, we could also define options in the source code (near the commands) instead of in separate .ini files. I don't see a benefit in having these .ini files separately. The options for a command could be defined in a hash table with name similar to the command, so that the respective option table can be found automatically when a command is seen. Or commands could be implemented in classes, and the class could a method returning the valid options.

I dislike a distinction between "sim --verbose submit" and "sim submit --verbose". If we reject the second, then we should should accompany it by an error message "please say 'sim --verbose submit' instead", and this seems a bit silly, since we may just interpret it this way in this case. Distinguishing between before-command and after-command options makes only sense if there are a lot of commands that have been designed independently, such that there are options that have different meanings depending on whether they come before or after the command (e.g. "cvs -d" comes to mind). I'd rather have a small set of options that is designed coherently (even if this requires lengthy discussions on the mailing list).

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain mthomas.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from mthomas to the specified user.
The owner will be changed from mthomas to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.