RFE: Realm: Tracking absent flags in the command line parser

[magnatelee]

Realm's command-line parser provides no way for clients to know which of the flags actually present in the command line. Knowing flags present in the command line would be helpful for the clients to use some sophisticated logic to fill in the missing values, as opposed to statically assigning default values. (technically, clients can identify missing values by making a specific value, say -1, signify the absence, but that isn't ergonomic.) An obvious, C++-way of supporting it is std::optional as a value type, which unfortunately needs C++17. Realm may consider rolling its own optional type for this purpose, which can be optionally backed by the standard implementation with C++>=17.

[streichler]

Wrapper types would be a bit harder here because the reference lifetimes are long. I think a first version that only exposes the capability for c++ >= 17 is simpler, and happens to be good enough for @magnatelee 's use case.

[elliottslaughter]

Looks like C++ 17 support was complete in GCC 8?

https://gcc.gnu.org/projects/cxx-status.html#cxx17

Given our current minimums https://github.com/StanfordLegion/legion/issues/1395, we could potentially raise the floor on the supported C++ standard.

[eddy16112]

After discussing with @streichler, I am proposing a c++>= 17 only API for the realm cmdline parser. Here is an example:

std::optional<int> opt_int(0);
CommandLineParser cp;
cp.add_option_int("-optint", opt_int);
bool ok = cp.parse_command_line(argc, const_cast<const char **>(argv));
assert(ok);

If the -optint presents, then opt_int will be set to the value from cmdline, otherwise, opt_int will be set to std::nullopt. Let me know it is enough, then I will start executing the plan.

[eddy16112]

@magnatelee we have discussed the optional support for command line parser. Here are two solutions:

1. add a new is_set API to query if a value is set

   int a;
   CommandLineParser cp;
   cp.add_option_int("-a", a);
   if (!cp.is_set("-a")) {} // not set

2. use std::optional

   std::optional<int> a;
   CommandLineParser cp;
   cp.add_option_int("-a", a);
   if (a == std::nullopt) {} // not set

   I am leaning towards 1 because using std::optional will bifurcate the API too much. Let me know your preference. add @apryakhin @muraj for visibility.

[magnatelee]

Let me know your preference.

My preference is option 2, as putting a value's presence close to the value itself would be the easiest way for users. I feel it's much easier for users to make mistakes with option 1.

using std::optional will bifurcate the API too much.

What does that mean? If I'm reading cmdline.h correctly, there's only one user-facing class (CommandLineParser). Those XXXCommandLineOption classes look like an implementation detail to me (as they are marked REALM_INTERNAL_API_EXTERNAL_LINKAGE), and I feel they could use some refactoring.

[eddy16112]

using std::optional will bifurcate the API too much.

We will need to refactor XXXCommandLineOption to support both T& and std::optional&, which is painful, otherwise, we will need a XXXCommandLineOption for T& and another OptionalXXXCommandLineOption for std::optional&, which brings a lot of duplicated code.

[magnatelee]

So I think the original sin was committed when the code was written such that it passes around the parsing destinations as references, instead of having XXXCommandLineOption return parsed values. if you guys don't have bandwidth to refactor this or don't want to maintain this as a user-facing API, that's fine. But I don't think I'm gonna make changes to Legate to use the API in option 2, because I don't think it's a good design.

[magnatelee]

Alternatively, we could build out a command-line parser support in Legate, as Legate programs would be the ones that have immediate needs for it.

[eddy16112]

So I think the original sin was committed when the code was written such that it passes around the parsing destinations as references, instead of having XXXCommandLineOption return parsed values.

Exactly, that is where the problem comes from. Given that all Legion applications could potentially use the command line parser API, it is hard to find a clean way to refactor the code without changing the existing API. If we do not care about the cleanness of the code, we could add the std::optional support in just 1-2 days.

[elliottslaughter]

Should we actually do a survey to find out how many applications use this API?

I'm not personally aware of any that do. So if the impact is small enough, we could just pull the plug and switch it.

[eddy16112]

I figured out a cleaner way to handle both &T and std::optional<T>& using the std::variant. Here is the PR. https://gitlab.com/StanfordLegion/legion/-/merge_requests/984

[magnatelee]

The PR looks okay if we insist on supporting both T and std::optional<T>, but I feel it'd be so much cleaner if we just used std::optional<T> exclusively. And I agree with Elliott that we should ask around and see if we could just switch to it.

[elliottslaughter]

Is there an easy link I can share to point users to the API we are considering changing?

[eddy16112]

If everyone agrees to move to std::optional<T>, I am more than happy to remove the support of T. Legion expose the command line parser to applications, so I guess most Legion applications use it, and if it is the case, it will be hard to deprecate the old T