StanfordLegion / legion

The Legion Parallel Programming System
https://legion.stanford.edu
Apache License 2.0
658 stars 146 forks source link

Typos in Legion runtime flags are unchecked #1045

Open elliottslaughter opened 3 years ago

elliottslaughter commented 3 years ago

Compare:

$ ./regent.py examples/circuit_sparse.rg -ll:cputypo 1
ERROR: unrecognized lowlevel option: -ll:cputypo

Vs:

$ ./regent.py examples/circuit_sparse.rg -lg:proftypo 1
ELAPSED TIME =   0.066 s
GFLOPS =   2.312 GFLOPS

Or for that matter, -lg:typo, -hl:typo, or variations thereof.

I noticed this while mistakenly trying to run -lg:no_trace_optimizations instead of -lg:no_trace_optimization (no s). I'm thankful @magnatelee pointed this out to me because I would have spent a long time looking for it otherwise.

It would be nice to have strict checks, particularly on the prefixed flags like -lg:, -hl:, -dm:, etc. Since Realm has this capability, maybe it can be exposed in a way that Legion might be able to reuse?

lightsighter commented 3 years ago

I'll make a trade with the group: I'll add support for errors on all -lg: flags that don't match if we can finally remove support for all -hl: flags. As for -dm: flags, I don't really care, that is a collective decision to be made by the group as well, but I will not be opinionated on it. @alexaiken to add to a future group meeting for discussion.

Arbing myself and @manopapad so he can take the -dm: flags to the mapper working group for discussion.

elliottslaughter commented 3 years ago

I'm fine with that tradeoff, -lg: has been supported for... years(?) now, so I think it's reasonable to remove -hl:.

manopapad commented 3 years ago

Note that Realm flags other than -ll: are also not checked, e.g. -gex:typo is accepted. The command line parser could check for typos in a general way by creating a list of prefixes used by all add_option commands and checking every unrecognized option against them.

elliottslaughter commented 2 months ago

I was surprised to find today that we still haven't done this and that -lg:typo 0 is not caught as an error.

I think Mike's suggestion in https://github.com/StanfordLegion/legion/issues/1045#issuecomment-814005483 was reasonable, should we actually do that?

elliottslaughter commented 2 months ago

We agreed in the meeting today to remove the old -hl: options.

@lightsighter to propose an API for the Realm command line parser to support checking unexpected flags in a given prefix.

lightsighter commented 1 month ago

Ok, I have created a draft interface for this in the Realm command line parser:

https://gitlab.com/StanfordLegion/legion/-/merge_requests/1312

Once we've got approval on that then I can update Legion. Did we decide we wanted to do the default mapper too?

elliottslaughter commented 1 month ago

If the API becomes available I can update the default mapper to use it as well.