[ETFeeder] Update attrs to optional

[changhai0109]

Summary

For those optional attrs in chakra schema, we introduce std::optional to store attr fields, which can be used to indicate whether a attr exists or not. Also add helper functions to check if has this attr, also try_get with default values.

Test Plan

# setup codes
git clone git@github.com:changhai0109/astra-sim.git astra-sim
cd astra-sim
git checkout changhai-improve-workload-layer
git submodule update --init --recursive
cd extern/graph/chakra
git remote add folk git@github.com:changhai0109/chakra.git
git fetch folk && git checkout changhai-improve-workload-layer
cd ../../..

# at astra-sim dir
bash build/astra_analytical/build.sh 
cd runs/example/workload
bash downlaod.sh
cd ..
bash run.sh

Then have output:

[2023-12-06 13:07:36.952] [topology::RingTopology] [info] ring of node 0, id: 0 dimension: local total nodes in ring: 64 index in ring: 0total nodes in ring: 1
[2023-12-06 13:07:36.952] [topology::RingTopology] [info] ring of node 0, id: 0 dimension: local total nodes in ring: 64 index in ring: 0total nodes in ring: 1
[2023-12-06 13:07:36.952] [topology::RingTopology] [info] ring of node 0, id: 0 dimension: local total nodes in ring: 64 index in ring: 0total nodes in ring: 1
[2023-12-06 13:07:36.952] [topology::RingTopology] [info] ring of node 0, id: 0 dimension: local total nodes in ring: 64 index in ring: 0total nodes in ring: 1
[2023-12-06 13:07:43.851] [workload] [info] sys[0] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[1] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[2] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[3] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[4] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[5] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[6] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[7] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[8] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[9] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[10] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[11] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[12] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[13] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[14] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[15] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[16] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[17] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[18] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[19] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[20] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[21] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[22] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[23] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[24] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[25] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[26] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[27] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[28] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[29] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[30] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[31] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[32] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[33] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[34] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[35] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[36] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[37] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[38] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[39] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[40] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[41] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[42] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[43] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[44] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[45] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[46] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[47] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[48] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[49] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[50] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[51] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[52] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[53] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[54] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[55] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[56] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[57] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[58] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[59] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[60] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[61] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[62] finished, 1069447000 cycles
[2023-12-06 13:07:43.851] [workload] [info] sys[63] finished, 1069447000 cycles
[2023-12-06 13:07:43.854] [System] [info] Exiting

[github-actions]

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

[TaekyungHeo]

Thank you for your contribution. First, as et_feeder had bugs that were fixed by another PR with a different design, it is unlikely that we can approve this PR as it stands. Second, I am not sure if there's a clear advantage to having the optional keyword in the et_feeder design. Let @srinivas212 decide.

[JoongunPark]

Thank you for your contribution. First, as et_feeder had bugs that were fixed by another PR with a different design, it is unlikely that we can approve this PR as it stands. Second, I am not sure if there's a clear advantage to having the optional keyword in the et_feeder design. Let @srinivas212 decide.

@TaekyungHeo
Changhai added the 'optional' keyword to minimize the empty memory space filled with random values. These not only degrade performance (both in compile and runtime) but also potentially introduce vulnerabilities.

@changhai0109
Taekyung has resolved some issues in the original code. If you think we still need 'optional' keyword, please update this PR!

[changhai0109]

@TaekyungHeo Thanks for your comments. I will check the PR #18 and migrate changes based on that.

@srinivas212 @JoongunPark
In a short word, the reason why "optional" needed is that, the current design cannot represent cases that the attr do not exist.

As we know an optional value has two states: {exist, not_exist}. The "required" field in ETFeeder can only handle "exist" cases, but not "not_exist" cases, as there should be no value of a "required" field should be mapped to a "not_exist" state since all possible values of a "required" should be valid value as a "exist" state for most basic datatypes like int or float. For example, with type int32_t, the optional filed has 2^32 possibile values in "exist" state, and 1 additional of "not_exist", however, required field can only fit 2^32 states.

In current design we assume the downstream user of ETFeeder has a set of rules that, they know for a certain type of node, which attrs should be there and which are not. However, that is just an assumption. We do not know who are going to use ETFeeder in their application (as chakra is open source, it do not have constraint to its user), and how they are going to use it.

@JoongunPark About the vulnerabilities, could you please show me how this change will introduce vulnerabilities. From my understanding, it does not introduce vulnerabilities, instead, it reduces the vulnerabilities as it elimitate a case of undefined behavior(user read non-exist attrs, and god know what they are reading).

[JoongunPark]

@TaekyungHeo Thanks for your comments. I will check the PR #18 and migrate changes based on that.

@srinivas212 @JoongunPark In a short word, the reason why "optional" needed is that, the current design cannot represent cases that the attr do not exist.

As we know an optional value has two states: {exist, not_exist}. The "required" field in ETFeeder can only handle "exist" cases, but not "not_exist" cases, as there should be no value of a "required" field should be mapped to a "not_exist" state since all possible values of a "required" should be valid value as a "exist" state for most basic datatypes like int or float. For example, with type int32_t, the optional filed has 2^32 possibile values in "exist" state, and 1 additional of "not_exist", however, required field can only fit 2^32 states.

In current design we assume the downstream user of ETFeeder has a set of rules that, they know for a certain type of node, which attrs should be there and which are not. However, that is just an assumption. We do not know who are going to use ETFeeder in their application (as chakra is open source, it do not have constraint to its user), and how they are going to use it.

@JoongunPark About the vulnerabilities, could you please show me how this change will introduce vulnerabilities. From my understanding, it does not introduce vulnerabilities, instead, it reduces the vulnerabilities as it elimitate a case of undefined behavior(user read non-exist attrs, and god know what they are reading).

Oh I what I meant was if we don't add 'optional' keyword, the 'empty memory space with random value' can introduce vulnerabilities. So basically we are saying the same thing.

"Changhai added the 'optional' keyword to minimize the empty memory space filled with random values. These not only degrade performance (both in compile and runtime) but also potentially introduce vulnerabilities."

[changhai0109]

Thank you for your contribution. First, as et_feeder had bugs that were fixed by another PR with a different design, it is unlikely that we can approve this PR as it stands. Second, I am not sure if there's a clear advantage to having the optional keyword in the et_feeder design. Let @srinivas212 decide.

@TaekyungHeo Thanks for your comments. I will check the PR #18 and migrate changes based on that.

@srinivas212 @JoongunPark In a short word, the reason why "optional" needed is that, the current design cannot represent cases that the attr do not exist.

As we know an optional value has two states: {exist, not_exist}. The "required" field in ETFeeder can only handle "exist" cases, but not "not_exist" cases, as there should be no value of a "required" field should be mapped to a "not_exist" state since all possible values of a "required" should be valid value as a "exist" state for most basic datatypes like int or float. For example, with type int32_t, the optional filed has 2^32 possibile values in "exist" state, and 1 additional of "not_exist", however, required field can only fit 2^32 states.

In current design we assume the downstream user of ETFeeder has a set of rules that, they know for a certain type of node, which attrs should be there and which are not. However, that is just an assumption. We do not know who are going to use ETFeeder in their application (as chakra is open source, it do not have constraint to its user), and how they are going to use it.

@JoongunPark About the vulnerabilities, could you please show me how this change will introduce vulnerabilities. From my understanding, it does not introduce vulnerabilities, instead, it reduces the vulnerabilities as it elimitate a case of undefined behavior(user read non-exist attrs, and god know what they are reading).

Hi, @srinivas212. Could you please decide whether we need optional attrs in ETFeederNode? Some recent astrasim PR are depending on this feature. If not, we need to find an alternative solution.