add command line parsing and print for #issue942

[lambda7xx]

Description of changes:

• add command line parsing and print for FFConfig
• add parser test
• support `--batch_size 100 -ll:gpus 4'
• test the batch size, it will throw exception, because we only support -- or - via command
• if we don't pass the required args via command like we don't pass the -ll:gpus, it will throw exceptions
• support -store_action. if it does pass via command, it will be false, otherwise, it will be true.
• --arg1 1 --arg2 --arg3 4 or --arg1 --arg2 4 --arg3 will throw exception because it miss to pass the args via command.

Related Issues:

Linked Issues:

• Issue # 942

Issues closed by this PR:

• Closes #

Before merging:

• Did you update the flexflow-third-party repo, if modifying any of the Cmake files, the build configs, or the submodules?

---

This change is 

[lockshaw]

Can you add an issue to also allow this to pull from a config file (e.g., yaml or json) and link it here?

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @lambda7xx)

---

lib/utils/include/utils/parse.h line 4 at r1 (raw file):

#define _FLEXFLOW_UTILS_INCLUDE_UTILS_PARSE_H

#include "runtime/config.h"

utils cannot depend on runtime

---

lib/utils/include/utils/parse.h line 12 at r1 (raw file):

namespace FlexFlow {

using VariantType = variant<int, bool, float, std::string>;

Suggestion:

using AllowedArgTypes = variant<int, bool, float, std::string>;

---

lib/utils/include/utils/parse.h line 14 at r1 (raw file):

using VariantType = variant<int, bool, float, std::string>;

class ArgsParser {

There should be a separate ArgsSpec or something which is a visitable object that contains a specification of the args that we can pass up to Python to plug into argparse

---

lib/utils/include/utils/parse.h line 18 at r1 (raw file):

  std::unordered_map<std::string, std::string> mArgs;
  std::unordered_map<std::string, VariantType> mDefaultValues;
  std::unordered_map<std::string, std::string> mDescriptions;

Potentially unify into a single unordered_map where the values are structs? As long as they should always have the same keyset this would help prevent bugs

Code quote:

  std::unordered_map<std::string, std::string> mArgs;
  std::unordered_map<std::string, VariantType> mDefaultValues;
  std::unordered_map<std::string, std::string> mDescriptions;

---

lib/utils/include/utils/parse.h line 20 at r1 (raw file):

  std::unordered_map<std::string, std::string> mDescriptions;

  std::string parseKey(std::string const &arg) const {

Convert to function (as there is no dependency on the class)

---

lib/utils/include/utils/parse.h line 24 at r1 (raw file):

      return arg.substr(2);
    } else {
      return arg;

Why is the -- here optional?

---

lib/utils/include/utils/parse.h line 30 at r1 (raw file):

public:
  ArgsParser() = default;
  void parse_args(int argc, char **argv) {

Move to .cc file

---

lib/utils/include/utils/parse.h line 47 at r1 (raw file):

                    VariantType const &value,
                    std::string const &description) {
    mDefaultValues[parseKey(key)] = std::move(value);

Suggestion:

    mDefaultValues[parseKey(key)] = value;

---

lib/utils/include/utils/parse.h line 54 at r1 (raw file):

  T get(std::string const &key) const {
    auto it = mArgs.find(key);
    if (it != mArgs.end()) {

contains from containers.h?

---

lib/utils/include/utils/parse.h line 67 at r1 (raw file):

  void showDescriptions() const {
    for (auto const &kv : mDescriptions) {
      std::cout << kv.first << ": " << kv.second << std::endl;

Also, this should probably use fmt or at the last use ostream's alignment options

Suggestion:

      std::cerr << kv.first << ": " << kv.second << std::endl;

---

lib/utils/include/utils/parse.h line 74 at r1 (raw file):

  T convert(std::string const &s) const;

  friend std::ostream &operator<<(std::ostream &out, ArgsParser const &args);

Use fmt for now

---

lib/utils/include/utils/parse.h line 89 at r1 (raw file):

template <>
bool ArgsParser::convert<bool>(std::string const &s) const {
  return s == "true" || s == "1";

Is ArgsParser currently case-sensitive for these values?

Suggestion:

  return s == "true" || s == "1" || s == "yes";

---

lib/utils/include/utils/parse.h line 116 at r1 (raw file):

    ArgsParser::get_from_variant<std::string>(VariantType const &v) const {
  return mpark::get<std::string>(v);
}

What is the purpose of these? It seems like they can all just be replaced by calling get directly?

At the very least they can be unified as suggested below

Suggestion:

template <typename T>
T ArgsParser::get_from_variant<T>(VariantType const &v) const {
  return mpark::get<int>(v);
}

---

lib/utils/include/utils/parse.h line 123 at r1 (raw file):

}

void FFConfig::parse_args(char **argv, int argc) {

This should go into runtime. utils/parse should just be a command-line parsing library with no explicit link to flexflow, and then runtime can use utils/parse for any flexflow-specific command line argument parsing

---

lib/utils/include/utils/parse.h line 193 at r1 (raw file):

  args.parse_args(argc, argv);

  batch_size = args.get<int>("batch-size");

It would be nice to use an enum or something for the names instead of strings so errors can be more easily detected at runtime. Right now it's really easily to accidentally create a typo in a string. For example,

Suggestion:

  auto batch_size_ref = args.add_argument("--batch-size", 32, "Size of each batch during training");
  batch_size = args.get(batch_size_ref); // type no longer needed as type can be inferred from batch_size_ref

---

lib/utils/test/src/test_parse.cc line 6 at r1 (raw file):

using namespace FlexFlow;

TEST_CASE("Test ArgsParser basic functionality") {

Add tests for invalid command line args

[lambda7xx]

OK

[lambda7xx]

lib/utils/include/utils/parse.h line 4 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

utils cannot depend on runtime

implement this in config.cc

[lambda7xx]

lib/utils/include/utils/parse.h line 24 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why is the -- here optional?

for example, I pass "--size", it parse it and get 'size'

[lambda7xx]

lib/utils/include/utils/parse.h line 193 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

It would be nice to use an enum or something for the names instead of strings so errors can be more easily detected at runtime. Right now it's really easily to accidentally create a typo in a string. For example,

I try to do that, but we can not. because there is two case

1. default value, its argument can be AllowedTypeValue
2. it parse the argc and argv to get a value, but the argv is a std::string, for the argc and argv, the compiler can't infer the type, we have pass its type.

so we have to pass args.get("xxx") to parse the command

the below are my code

Code snippet:

  ArgRef add_argument(std::string const &key,
                    AllowedArgTypes const &value,
                    std::string const &description) const {
    mDefaultValues[parseKey(key)] = value;
    mDescriptions[key] = description; 
    return ArgRef{key, value};
  }
  
    void parse_args(int argc, char **argv) {
    for (int i = 1; i < argc; i += 2) {
      std::string key = parseKey(argv[i]);
      if (key == "help" || key == "h") {
        showDescriptions();
        exit(0);
      }
      mArgs[key] = argv[i + 1];//its std::string
    }
  }

[lambda7xx]

lib/utils/include/utils/parse.h line 193 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

I try to do that, but we can not. because there is two case

1. default value, its argument can be AllowedTypeValue
2. it parse the argc and argv to get a value, but the argv is a std::string, for the argc and argv, the compiler can't infer the type, we have pass its type.

so we have to pass args.get("xxx") to parse the command

the below are my code

we still need pass the type. the below is my implementation.

Code snippet:

//parse.h 
#ifndef _FLEXFLOW_UTILS_INCLUDE_UTILS_PARSE_H
#define _FLEXFLOW_UTILS_INCLUDE_UTILS_PARSE_H

//#include "runtime/config.h"
#include "utils/exception.h"
#include "utils/variant.h"
#include "utils/containers.h"
#include <ostream>
#include <string>
#include <unordered_map>
namespace FlexFlow {

using AllowedArgTypes = variant<int, bool, float, std::string>;

struct ArgRef {
  std::string key;
  AllowedArgTypes value;
};

enum ArgTypes{
  INT,
  BOOL,
  FLOAT,
  STRING,
};

std::string parseKey(std::string arg)  {
    if (arg.substr(0, 2) == "--") {
      return arg.substr(2);
    } else {
      return arg;
    }
}

class ArgsParser {
private:
  std::unordered_map<std::string, AllowedArgTypes> mArgs;
  std::unordered_map<std::string, ArgTypes> mArgTypes; 
  std::unordered_map<std::string, std::string> mDescriptions;

public:
  ArgsParser() = default;
  void parse_args(int argc, char **argv) {
    for (int i = 1; i < argc; i += 2) {
      std::string key = parseKey(argv[i]);
      if (key == "help" || key == "h") {
        showDescriptions();
        exit(0);
      }
      if(!contains_key(mArgs, key)) {
        throw mk_runtime_error("Key not found: " + key);
      }

      ArgTypes arg_type = mArgTypes[key];
      switch(arg_type) {
        case INT:
          mArgs[key] = std::stoi(argv[i + 1]);
          break;
        case BOOL:
          mArgs[key] = std::stoi(argv[i + 1]);
          break;
        case FLOAT:
          mArgs[key] = std::stof(argv[i + 1]);
          break;
        case STRING:
          mArgs[key] = argv[i + 1];
          break;
        default:
          throw mk_runtime_error("Key not found: " + key);
      }
    }
  }

  ArgRef add_argument(std::string const &key,
                    AllowedArgTypes const &value,
                    std::string const &description)  {
    std::string  parse_key = parseKey(key);
    mArgs[parse_key] = value;
    switch(value.index()) {
      case 0:
        mArgTypes[parse_key] = INT;
        break;
      case 1:
        mArgTypes[parse_key] = BOOL;
        break;
      case 2:
        mArgTypes[parse_key] = FLOAT;
        break;
      case 3:
        mArgTypes[parse_key] = STRING;
        break;
      default:
        throw mk_runtime_error("Key not found: " + key);
    }
    mDescriptions[key] = description; 
    return ArgRef{key, value};
  }

  template <typename T>
  T get_from_variant(AllowedArgTypes const &v) const;

  AllowedArgTypes get(ArgRef const & ref) const;
    // auto it = mArgs.find(key);
    // if (it != mArgs.end()) {
    //   return 
    // } else {
    //   auto def_it = mDefaultValues.find(key);
    //   if (def_it != mDefaultValues.end()) {
        
    //   }
    // }
    // throw mk_runtime_error("Key not found: " + key);
  
  // template <typename T>
  // T get(std::string const &key) const {
  //   auto it = mArgs.find(key);
  //   if (it != mArgs.end()) {
  //     return convert<T>(it->second);
  //   } else {
  //     auto def_it = mDefaultValues.find(key);
  //     if (def_it != mDefaultValues.end()) {
  //       return get_from_variant<T>(def_it->second);
  //     }
  //   }
  //   throw mk_runtime_error("Key not found: " + key);
  // }

  void showDescriptions() const {
    for (auto const &kv : mDescriptions) {
      std::cout << kv.first << ": " << kv.second << std::endl;
    }
  }

 // AllowedArgTypes convert(std::string const &s) const;

  friend std::ostream &operator<<(std::ostream &out, ArgsParser const &args);
};


std::ostream &operator<<(std::ostream &out, ArgsParser const &args) {
  args.showDescriptions();
  return out;
}




} // namespace FlexFlow

#endif

//parse.cc 

#include "utils/parse.h"
#include "utils/containers.h"

namespace FlexFlow{

template <>
int ArgsParser::get_from_variant<int>(AllowedArgTypes const &v) const {
  return mpark::get<int>(v);
}

template <>
float ArgsParser::get_from_variant<float>(AllowedArgTypes const &v) const {
  return mpark::get<float>(v);
}

template <>
bool ArgsParser::get_from_variant<bool>(AllowedArgTypes const &v) const {
  return mpark::get<bool>(v);
}

template <>
std::string
    ArgsParser::get_from_variant<std::string>(AllowedArgTypes const &v) const {
  return mpark::get<std::string>(v);
}

  AllowedArgTypes ArgsParser::get(ArgRef const & ref) const {
    std::string key =parseKey(ref.key); 
    if(!contains_key(mArgs, key)) {
      throw mk_runtime_error("invalid args: " + ref.key);
    }
    AllowedArgTypes value = mArgs.at(key);
    ArgTypes arg_type = mArgTypes.at(key);
    switch(arg_type) {
      case INT:
        return get_from_variant<int>(value);
      case BOOL:
        return get_from_variant<bool>(value);
      case FLOAT:
        return get_from_variant<float>(value);
      case STRING:
        return get_from_variant<std::string>(value);
      default:
        throw mk_runtime_error("Key not found: " + key);
    }
  }

} // namespace FlexFlow

//test_parse.cc 
#include "doctest.h"
#include "utils/parse.h"

using namespace FlexFlow;

TEST_CASE("Test ArgsParser basic functionality") {
  char const *test_argv[] = {"program_name",
                             "--batch-size",
                             "100",
                             "--learning-rate",
                             "0.5",
                             "--fusion",
                             "true",
                             "-ll:gpus",
                             "6"};
  ArgsParser args;
  auto batch_size_ref = args.add_argument(
      std::string("--batch-size"), 32, std::string("Size of each batch during training"));
  auto learning_rate_ref = args.add_argument(
      "--learning-rate", 0.01f, "Learning rate for the optimizer");
  auto fusion_ref = args.add_argument(
      "--fusion",
      false,
      "Flag to determine if fusion optimization should be used");
  auto ll_gpus_ref = args.add_argument(
      "-ll:gpus", 2, "Number of GPUs to be used for training");
  args.parse_args(9, const_cast<char **>(test_argv));
  std::cout<<args.get_from_variant<int>(args.get(batch_size_ref))<<std::endl; // it will fail
 // CHECK(args.get(batch_size_ref)== 100);
  // CHECK(args.get(learning_rate_ref) == 0.5f);
  // CHECK(args.get(fusion_ref) == true);
  // CHECK(args.get(ll_gpus_ref) == 6);
}

[lambda7xx]

lib/utils/include/utils/parse.h line 116 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What is the purpose of these? It seems like they can all just be replaced by calling get directly?

At the very least they can be unified as suggested below

You mean ?

Code snippet (i):

template <typename T>
  T ArgsParser::get_from_variant(AllowedArgTypes const &v) const {
    return mpark::get<T>(v);
  }

Code snippet (ii):

I fix it.

[lambda7xx]

lib/utils/include/utils/parse.h line 74 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Use fmt for now

these commit are far behind the latest, maybe I should set a separate PR to use fmt after this PR is merged.

[lambda7xx]

I reply in other channel.

Reviewable status: 1 of 4 files reviewed, 16 unresolved discussions (waiting on @lockshaw)

---

lib/utils/include/utils/parse.h line 12 at r1 (raw file):

namespace FlexFlow {

using VariantType = variant<int, bool, float, std::string>;

Done.

---

lib/utils/include/utils/parse.h line 20 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Convert to function (as there is no dependency on the class)

Done.

---

lib/utils/include/utils/parse.h line 30 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Move to .cc file

Done.

---

lib/utils/include/utils/parse.h line 47 at r1 (raw file):

                    VariantType const &value,
                    std::string const &description) {
    mDefaultValues[parseKey(key)] = std::move(value);

Done.

---

lib/utils/include/utils/parse.h line 54 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

contains from containers.h?

Done.

---

lib/utils/include/utils/parse.h line 67 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Also, this should probably use fmt or at the last use ostream's alignment options

Done.

---

lib/utils/include/utils/parse.h line 89 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Is ArgsParser currently case-sensitive for these values?

Done.

[lockshaw]

Can you add a link to the new issue here?

Reviewed 3 of 3 files at r2, 1 of 4 files at r3.
Reviewable status: 2 of 5 files reviewed, 15 unresolved discussions (waiting on @lambda7xx)

---

lib/runtime/src/config.cc line 6 at r2 (raw file):

namespace FlexFlow {

// void FFConfig::parse_args(char **argv, int argc) {

Why is this all commented out?

---

lib/utils/include/utils/parse.h line 24 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

for example, I pass "--size", it parse it and get 'size'

Yes, but parseKey("size") also equals "size", so wouldn't ./flexflow size 10 and ./flexflow --size 10 be treated identically (when one is clearly wrong)?

---

lib/utils/include/utils/parse.h line 54 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

Done.

Did this code disappear? I'm not seeing it

---

lib/utils/include/utils/parse.h line 74 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

these commit are far behind the latest, maybe I should set a separate PR to use fmt after this PR is merged.

I'm okay with that--can you create an issue and link it here?

---

lib/utils/include/utils/parse.h line 89 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

Done.

I don't see the case sensitivity handled, and I don't see test cases for this

---

lib/utils/include/utils/parse.h line 116 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

You mean ?

Or just get rid of the method entirely and just call get directly

---

lib/utils/include/utils/parse.h line 193 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

we still need pass the type. the below is my implementation.

The key is to make ArgRef (also, rename to CmdlineArgRef for clarity) actually a template <typename T> struct ArgRef; so that you can get the type back again when you parse--that should fix the type inference problem

---

lib/utils/include/utils/parse.h line 23 at r2 (raw file):

class ArgsParser {
private:
  std::unordered_map<std::string, std::string> mArgs;

Why not just use a single map?

---

lib/utils/include/utils/parse.h line 29 at r2 (raw file):

public:
  ArgsParser() = default;
  void parse_args(int argc, char **argv);

Where did the implementation of this go?

---

lib/utils/src/parse.cc line 41 at r3 (raw file):

      std::cerr  << kv.first << ": " << kv.second << std::endl;
    }
  }

(also, formatting)

Suggestion:

void ArgsParser::showDescriptions() const {
    std::cerr << *this << std::endl;
  }

---

lib/utils/src/parse.cc line 47 at r3 (raw file):

  args.showDescriptions();
  return out;
}

Suggestion:

std::string format_as(ArgsParser const &args) {
  ... 
}

[lambda7xx]

the issue is 942?

[lambda7xx]

lib/runtime/src/config.cc line 6 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why is this all commented out?

it seems I remove the commented

[lambda7xx]

lib/utils/include/utils/parse.h line 18 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Potentially unify into a single unordered_map where the values are structs? As long as they should always have the same keyset this would help prevent bugs

I think three is ok. one is for help, one is for default and other one is for args.

[lambda7xx]

lib/utils/include/utils/parse.h line 14 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

There should be a separate ArgsSpec or something which is a visitable object that contains a specification of the args that we can pass up to Python to plug into argparse

i am a little confused about your words.

you mean we should use FF like. FF_STRUCT_VISITABLE for ArgsSpec

[lambda7xx]

lib/utils/include/utils/parse.h line 24 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Yes, but parseKey("size") also equals "size", so wouldn't ./flexflow size 10 and ./flexflow --size 10 be treated identically (when one is clearly wrong)?

I think my implementation support --size and size.

[lambda7xx]

lib/utils/include/utils/parse.h line 23 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why not just use a single map?

because we have default value and non-default value.

[lambda7xx]

lib/utils/include/utils/parse.h line 29 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Where did the implementation of this go?

it's for help. when user parse help, we just print all argument.

[lambda7xx]

lib/utils/include/utils/parse.h line 54 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Did this code disappear? I'm not seeing it

I will push. in my code, I use contains from. containers.h

[lambda7xx]

lib/utils/include/utils/parse.h line 74 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I'm okay with that--can you create an issue and link it here?

ok

[lambda7xx]

Reviewable status: 2 of 5 files reviewed, 15 unresolved discussions (waiting on @lockshaw)

---

lib/utils/include/utils/parse.h line 123 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This should go into runtime. utils/parse should just be a command-line parsing library with no explicit link to flexflow, and then runtime can use utils/parse for any flexflow-specific command line argument parsing

Done.

[lambda7xx]

lib/utils/src/parse.cc line 47 at r3 (raw file):

  args.showDescriptions();
  return out;
}

i set a new issue for it

[lambda7xx]

lib/utils/include/utils/parse.h line 116 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Or just get rid of the method entirely and just call get directly

i think we shoud use mpark::get, otherwise the compiler maybe confused which get to use. because the class has a get method

[lambda7xx]

Reviewable status: 1 of 5 files reviewed, 15 unresolved discussions (waiting on @lockshaw)

---

lib/utils/include/utils/parse.h line 89 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I don't see the case sensitivity handled, and I don't see test cases for this

Done.

---

lib/utils/include/utils/parse.h line 193 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

The key is to make ArgRef (also, rename to CmdlineArgRef for clarity) actually a template <typename T> struct ArgRef; so that you can get the type back again when you parse--that should fix the type inference problem

Done.

---

lib/utils/src/parse.cc line 41 at r3 (raw file):

Previously, lockshaw (Colin Unger) wrote…

(also, formatting)

Done.

---

lib/utils/test/src/test_parse.cc line 6 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Add tests for invalid command line args

Done.

[lockshaw]

Reviewed 1 of 4 files at r3, 3 of 3 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @lambda7xx)

---

lib/runtime/src/config.cc line 6 at r4 (raw file):

namespace FlexFlow {

void FFConfig::parse_args(char **argv, int argc) {

Suggestion:

FFConfig parse_args(char **argv, int argc) {

---

lib/runtime/src/config.cc line 51 at r4 (raw file):

  args.add_argument("-level", 5, "level of logging output");
  args.add_argument("-logfile", "", "name of log file");
  args.add_argument("-ll:cpu", 1, "CPUs per node");

Suggestion:

  args.add_argument("-level", 5, "level of logging output");
  args.add_argument("-logfile", std::nullopt, "name of log file");
  args.add_argument("-ll:cpu", 1, "CPUs per node");

---

lib/runtime/src/config.cc line 52 at r4 (raw file):

  args.add_argument("-logfile", "", "name of log file");
  args.add_argument("-ll:cpu", 1, "CPUs per node");
  args.add_argument("-ll:gpu", 0, "GPUs per node");

This argument should be mandatory

Suggestion:

  args.add_argument("-ll:gpu", "GPUs per node");

---

lib/runtime/src/config.cc line 77 at r4 (raw file):

  batch_size = args.get<int>("batch-size");
  epochs = args.get<int>("epochs");

Suggestion:

return FFConfig{
  // ...
  args.get(epochs);
  // ...
};

---

lib/utils/include/utils/parse.h line 14 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

i am a little confused about your words.

you mean we should use FF like. FF_STRUCT_VISITABLE for ArgsSpec

We need to be able to pass a constant, data-only expression up through the FFI to argparse which represents the CLI signature we've defined here so that it can be integrated into the Python ffi's CLI

(when I say visitable, I generally mean a data-only type)

---

lib/utils/include/utils/parse.h line 18 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

I think three is ok. one is for help, one is for default and other one is for args.

This just creates more opportunities for errors--let's use one map

---

lib/utils/include/utils/parse.h line 24 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

I think my implementation support --size and size.

I know: it should only support --size, not size -- supporting both is incorrect

---

lib/utils/include/utils/parse.h line 74 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

ok

Please link the issue here

---

lib/utils/include/utils/parse.h line 89 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

Done.

I still don't see test cases for case sensitivity

---

lib/utils/include/utils/parse.h line 116 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

i think we shoud use mpark::get, otherwise the compiler maybe confused which get to use. because the class has a get method

That's fine, you just don't need ArgsParser::get_from_variant

---

lib/utils/include/utils/parse.h line 23 at r2 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

because we have default value and non-default value.

If they're all going to have the same keys, convert to a single unordered_map

---

lib/utils/include/utils/parse.h line 43 at r4 (raw file):

  template <typename T>
  T get(std::string const &key) const {
    if (contains_key(mArgs, key)) {

Why not just overwrite the value rather than keeping around two separate dictionaries?

---

lib/utils/src/parse.cc line 20 at r4 (raw file):

    if (key == "help" || key == "h") {
      showDescriptions();
      exit(0);

Suggestion:

      exit(1);

---

lib/utils/src/parse.cc line 56 at r4 (raw file):

}

// TODO(lambda):in the future, we will use fmt to format the output

Please link the issue in the code

Code quote:

// TODO(lambda):in the future, we will use fmt to format the output

---

lib/utils/test/src/test_parse.cc line 6 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

Done.

I still don't see any invalid arguments in test_argv

[lambda7xx]

lib/utils/include/utils/parse.h line 24 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I know: it should only support --size, not size -- supporting both is incorrect

so, if we pass the size, it will be error and throw an exceptions?

[lockshaw]

Reviewable status: 0 of 5 files reviewed, 15 unresolved discussions (waiting on @lambda7xx and @wmdi)

---

lib/utils/include/utils/parse.h line 24 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

so, if we pass the size, it will be error and throw an exceptions?

Yes (ideally if used interactively it would print an error message, but just throwing an exception is fine for now)

[lockshaw]

I'm still seeing lots of open comments--can you go through and mark the ones you think you've resolved as "Done" and try to resolve any that aren't currently fixed?

Reviewed all commit messages.
Reviewable status: 0 of 5 files reviewed, 15 unresolved discussions (waiting on @lambda7xx and @wmdi)

[lambda7xx]

lib/utils/include/utils/parse.h line 74 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Please link the issue here

Done. Implement command line parsing and help printing · Issue #942 · flexflow/FlexFlow (github.com)

[lockshaw]

Superseded by #1490