chakra icon indicating copy to clipboard operation
chakra copied to clipboard

In et_generator, change 'comm_size' attr from uint64 to int64

Open jinsun-yoo opened this issue 5 months ago • 4 comments

Summary

This PR makes et_generator use int64, instead of uint64 for 'comm_size' when creating collective nodes. et_feeder parses comm_size as a int64 datatype(et_feeder.cpp:28). With the current strong datatype conversion, using a uint64 value would make attr.int64_val() parse this value as 0 (appendix 1)

Also added static_cast into uint64_t for comm_size, which #18 seems to have missed. (@TaekyungHeo , please confirm if the lack of static_cast was intentional)

Test Plan

Created & ran the following test locally, to verify that et_generator using uint64 causes comm_size to be parsed as 0. I did not add this test into the PR, as this is only used to verify the problem statement, not any of our code.

Generate a test ET using the following code

comm_size = 212480
output_filename = "chakra.1.et"
with open(output_filename, "wb") as et:
    encode_message(et, GlobalMetadata(version="0.0.4"))

    node = get_node("node_with_int64", COMM_COLL_NODE)
    node.attr.append(ChakraAttr(name="comm_size", int64_val=comm_size))
    encode_message(et, node)

    node = get_node("node_with_uint64", COMM_COLL_NODE)
    node.attr.append(ChakraAttr(name="comm_size", uint64_val=comm_size))
    encode_message(et, node)

To generate the following trace

{
  "version": "0.0.4"
}{
  "name": "node_with_int64",
  "type": "COMM_COLL_NODE",
  "attr": [
    {
      "name": "comm_size",
      "int64Val": "212480"
    }
  ]
}{
  "id": "1",
  "name": "node_with_uint64",
  "type": "COMM_COLL_NODE",
  "attr": [
    {
      "name": "comm_size",
      "uint64Val": "212480"
    }
  ]
}

To be tested with the following test


TEST_F(ETFeederTest, ShowUint64DoesNotWorkTest) {
  SetUp("tests/data/chakra.1.et");
  std::shared_ptr<Chakra::ETFeederNode> node;

  node = trace->lookupNode(0);
  uint64_t comm_size = node->comm_size();
  ASSERT_EQ(comm_size, 212480);

  node = trace->lookupNode(1);
  comm_size = node->comm_size();
  ASSERT_EQ(comm_size, 0);
}

Additional Notes

Appendix 1) From et_def.pb.h:

inline ::google::protobuf::int64 AttributeProto::int64_val() const {
  // @@protoc_insertion_point(field_get:ChakraProtoMsg.AttributeProto.int64_val)
  if (has_int64_val()) {
    return value_.int64_val_;
  }
  return GOOGLE_LONGLONG(0);
}

jinsun-yoo avatar Sep 01 '24 23:09 jinsun-yoo