567 improved arg handling #662

lifflander · 2020-02-03T20:40:25Z

Basically:

All arguments must be accounted for and correctly parsed.
This mitigates "oops" cases when arguments are not correctly
handled by appropriate domain.

To supply MPI_Init args (still does not address issue of late MPI_Init), use:
--vt_mpi_args mpiarg1 mpiarg2..
To supply user args (returned up through initialize):
-- userarg1 userarg2 ..
To switch back to VT args, use --vt_args
Also supports 'auto pass-through' args for historic reasons.
That is, any arguments before a "--vt_" arg including "--vt_help"
are automatically classified as a pass-through arguments and any
"--vt_" or argument will implicitly switch into vt-arg mode.

1 2 3 --vt_no_color is the same as
--vt_no_color -- 1 2 3 while
--vt_no_color 1 2 3 is invalid (left in vt-arg context where such are invalid).

Most notable, short vt args like '-c' or '-q' do NOT initiate
vt-arg mode and will subject to pass-through behavior.
(Short args have been removed to avoid this edge-case.)
The syntax can also be extended indefinitely (although auto-mode only applies once at the start), for whatever use it may have:
1 --vt_no_color -- 2 --vt_args --vt_trace -- 3 is the same as
1 2 3 --vt_no_color --vt_trace and
--vt_no_color --vt_trace -- 1 2 3

It is recommended that parameters that take values do so with the --param=value form.

This addresses some PEBCAK errors locally and provides a base for some future changes with directory arg handling. There is a chance similar issues are accidentally encountered on other systems.

Pass-through arguments (and the program name) are displayed in the banner for clarity.

It is a "breaking" change in that some previously valid parameters invocations are not forbidden, ie. needing to use a -- to switch out of vt-arg context. However changes required are an artifact of the parameters supplied during invocation (as opposed to requiring code changes).

When in vt-arg context, unknown arguments will abort the program with an argument error.

The --vt_help (replaces --help) parameter is also more useful (and program-aborting).

Fixes #567

pnstickne

@lifflander
Looks about as left.

Re-mention of notable differences:

Removal of several short-args
VT termination on UNKNOWN VT-args (that is, arguments that are completely not valid for the build, such a requesting tracing when the build does not support tracing); the short-args which are removed now count as UNKNOWN.
..and a help message?

The Codacy issue on the overload is a false-positive (it's a bad rule, there should never be a "suggestion" to fix the type of an overload) and should be suppressed. I was not able to do so manually and there does not appear to be any comment-style disables.

pnstickne · 2020-02-04T03:32:40Z

Oh, hmm, hmm, conflicts what..

pnstickne · 2020-02-04T04:11:15Z

Rebased onto origin/develop.

codecov · 2020-02-04T04:43:33Z

Codecov Report

Merging #662 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #662   +/-   ##
========================================
  Coverage    80.58%   80.58%           
========================================
  Files          354      354           
  Lines        11136    11136           
========================================
  Hits          8974     8974           
  Misses        2162     2162

Impacted Files	Coverage Δ
src/vt/configs/arguments/args.h	`100.00% <ø> (ø)`
src/vt/runtime/runtime.h	`100.00% <ø> (ø)`

pnstickne · 2020-02-04T05:13:35Z

Well, Codacy is a [getting it wrong] thing here.. :|

lifflander · 2020-04-21T04:29:46Z

@pnstickne This is my fault, but this has gotten delayed. Do we still want to merge this? Can you rebase it if so?

- All banner, no runtime logic. Now has it's own little home.

- One function doing too much and mixing stuff. - Also fixes code to return 'exit code'; ie. 0 = success, other = exit.

- All arguments must be accounted for and correctly parsed. This mitigates "oops" cases when arguments are not correctly handled by appropriate domain. To supply MPI_Init args, use: `--vt_mpi_args mpiarg1 mpiarg2..` To supply user args (returned up through initialize): `-- userarg1 userarg2 ..` To switch back to VT args, use `--vt_args` - Also supports 'auto pass-through' args for historic reasons. That is, any arguments before a "--vt_*" arg or "--help" are automatically classified as a pass-through arguments. `1 2 3 --vt_no_color` is the same as `--vt_no_color -- 1 2 3` while `--vt_no_color 1 2 3` is invalid (left in vt-arg context). Most notable, short vt args like '-c' or '-q' do NOT initiate vt-arg mode and will subject to pass-through behavior. args mergeup

- VT now terminates immediately on non-accounted-for arguments; and when --help is requested. Before args could be 'like whatever' and the help banner just flew through the terminal. - Mitigates / controls some of arg-ref behavior, builds a clean MPI_Init argument set, and removes confusing user_argc/user_argv. - Uses the new exposed prog_name which is also guaranteed to no contain extra pathing info..

- Banner message includes additional useful information about arguments that are passed downsteam. Arguments passed downsteam are not used by VT and coupled with the stricter argument checking.. should be more friendly! - Banner includes the program name (just-file-name and any more complete path passed down from mpirun).

- A little bit of shifting now to help a little bit more in the future.

- All calls now go through sync(), instead of some through there and some through MPI_Barrier.

- If any more is added, it should be skipped as parsing has been trivially rejected at the lower level. (In the future there may be more immediate post-parsing actions.)

- Now generates an MPI_Init/MPI_Abort sandwich to ensure basic contract of MPI. Output is only emitted during these times; the programs still terminate immediately thereafter.

- Help is now "--vt_help" - Removed short -c/-n/-q options for output. Use the full names like "--vt_quiet" instead.

- To avoid excessive spam, only the first node now emits a message. The responsibility is also pushed out. If the abort was caused by "help" the message is now written to stdout (instead of stderr) and the result code is 0. This is more uniform with many CLI utilities as the displaying of the help message, which was requested, is successful.

- Now the parsing and configuration are separate types, even if still sharing the same cc file for declarations. The primary reason is to reduce header-bleed of implementation related types (such as vector<int,string>). This also force uniform access to ArgConfig for all.. Also makes future non-static conversion of ArgParse much more.. lucrative. (eg. supply result message as member instead of returning outward)

- Added description and synopsis covering argument modes, switching between modes, and usages of such.. - Also unifies naming including that in banner messages. - "VT args" - used by VT configuration; --vt_* - "Application args" - supplied down-stream as argv - "MPI args" - used for MPI Init

- std::_Exit is a little more 'abrupt' than C's exit in that it does less flushing and will not call registered at-exit's.. I guess, consistency is better? Maybe.

lifflander · 2020-06-11T17:29:36Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1
- Added 1

See the complete overview on Codacy

lifflander · 2020-06-11T17:29:36Z

src/vt/configs/arguments/args.cc

-    return app.exit(ex);
+class VtFormatter : public CLI::Formatter {
+public:
+  std::string make_usage(const CLI::App *, std::string name) const override {


Issue found: Function parameter 'name' should be passed by const reference.

pnstickne approved these changes Feb 4, 2020

View reviewed changes

pnstickne force-pushed the 567-improved-arg-handling branch from 786cfdb to 0182564 Compare February 4, 2020 04:10

pnstickne force-pushed the 567-improved-arg-handling branch from 0182564 to 429c270 Compare February 4, 2020 16:58

pnstickne assigned lifflander Feb 4, 2020

pnstickne added the 1.1.0 label Feb 4, 2020

pnstickne force-pushed the 567-improved-arg-handling branch from 429c270 to 6cdeed2 Compare March 1, 2020 20:40

lifflander force-pushed the 567-improved-arg-handling branch from 6cdeed2 to 31bb849 Compare June 11, 2020 16:37

pnstickne and others added 15 commits June 11, 2020 09:39

#576 move runtime banners to runtime_banner.cc

3401ece

- All banner, no runtime logic. Now has it's own little home.

#576 split arguments from parsing in args.cc

7a3ec1f

- One function doing too much and mixing stuff. - Also fixes code to return 'exit code'; ie. 0 = success, other = exit.

#576 split arg building into separate functions

9ffb39d

- A little bit of shifting now to help a little bit more in the future.

#576 use sync uniformly in Runtime class

8cf4039

- All calls now go through sync(), instead of some through there and some through MPI_Barrier.

#576 non-success arg parsing skips all post-handling

1acfb13

- If any more is added, it should be skipped as parsing has been trivially rejected at the lower level. (In the future there may be more immediate post-parsing actions.)

#576 honor MPI_Init contract before IO/term

598e0f1

- Now generates an MPI_Init/MPI_Abort sandwich to ensure basic contract of MPI. Output is only emitted during these times; the programs still terminate immediately thereafter.

#576 remove non-"--vt_" args

ffbd4b0

- Help is now "--vt_help" - Removed short -c/-n/-q options for output. Use the full names like "--vt_quiet" instead.

#576 switch to std::_Exit for consistency

e237ab6

- std::_Exit is a little more 'abrupt' than C's exit in that it does less flushing and will not call registered at-exit's.. I guess, consistency is better? Maybe.

#567: args: fix code from bad merge

c50c524

lifflander force-pushed the 567-improved-arg-handling branch from 31bb849 to c50c524 Compare June 11, 2020 16:39

pnstickne approved these changes Jun 11, 2020

View reviewed changes

#567: args: fix leaks causing ASAN failure

4c78b3f

lifflander commented Jun 11, 2020

View reviewed changes

lifflander merged commit 00dbd40 into develop Jun 11, 2020

cz4rs mentioned this pull request Jun 15, 2021

move vt version number into a separate file #1467

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

567 improved arg handling #662

567 improved arg handling #662

lifflander commented Feb 3, 2020

pnstickne left a comment

pnstickne commented Feb 4, 2020

pnstickne commented Feb 4, 2020

codecov bot commented Feb 4, 2020 •

edited

Loading

pnstickne commented Feb 4, 2020

lifflander commented Apr 21, 2020

lifflander commented Jun 11, 2020

lifflander Jun 11, 2020

567 improved arg handling #662

567 improved arg handling #662

Conversation

lifflander commented Feb 3, 2020

pnstickne left a comment

Choose a reason for hiding this comment

pnstickne commented Feb 4, 2020

pnstickne commented Feb 4, 2020

codecov bot commented Feb 4, 2020 • edited Loading

Codecov Report

pnstickne commented Feb 4, 2020

lifflander commented Apr 21, 2020

lifflander commented Jun 11, 2020

lifflander Jun 11, 2020

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2020 •

edited

Loading