feat(iota-core): update neural network model server from congestion tracker module #8993
Conversation
Signed-off-by: salaheldinsoliman <[email protected]>
Signed-off-by: salaheldinsoliman <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 6 Skipped Deployments
|
Signed-off-by: salaheldinsoliman <[email protected]>
salaheldinsoliman
requested review from
vekkiokonio
and removed request for
a team
| ) { | ||
| // 1) Build touched set | ||
| let mut touched: std::collections::HashSet<ObjectID> = std::collections::HashSet::new(); | ||
| for tx in congestion_txs_data { |
There was a problem hiding this comment.
Here you don't differentiate between write shared objects, read shared objects and owned objects. That means that read-only or owned objects would affect the prediction, which shouldn't be the case
There was a problem hiding this comment.
For instance, check the filter at line 327
There was a problem hiding this comment.
@salaheldinsoliman I think my comment was wrong here: you are using the same congestion_txs_data and cleared_txs_data from process_checkpoint_effect() so there is actually no need to include the mutate filter for congested objects -- in fact, we can only have congestion for write shared objects. I suggest to revert ddb1243
| } | ||
| } | ||
|
|
||
| // 3) Build per-checkpoint stats |
There was a problem hiding this comment.
Both 2) and 3) are "per-checkpoint stats". The difference is that 2) is retrieved directly from congestion_info, while 3) is computed in this function. Stats in 2) provide richer information as they also include gas prices. I'm wondering if stats in 3) are really needed and if they actually contributed to improve the NN prediction.
There was a problem hiding this comment.
Good point 👍
Step (2) is a post‑checkpoint snapshot from the cache (latest_* times, gas‑price extremes, hotness),
while step (3) adds per‑checkpoint intensity: how many congested/clearing txs touched the object. The booleans in (3) are redundant (derivable from (2) by comparing latest_*_time to this checkpoint), but the counts are not and provide extra signal for the predictor.
I think we should keep the counts for now, WDYT?
There was a problem hiding this comment.
Sure, there is no harm to keep them
| gas_price_feedback: None, | ||
| touched_objects: tx.objects.clone(), | ||
| }); | ||
| for oid in &tx.objects { |
There was a problem hiding this comment.
Your per_obj_min_clearing vector corresponds to get_prediction_suggested_gas_price computed for all objects in the checkpoint. However, your code doesn't handle the rare but possible case where no clearing data are present and all transactions are canceled for a given checkpoint
There was a problem hiding this comment.
nice catch, what I did now is basically:
A second accumulator that tracks, per object, the highest congestion requirement seen in the checkpoint (using gas_price_feedback if present, otherwise the gas price,).
After the single pass over transactions, I merge the two into a per_obj_required_in_cp map: it prefers the lowest clearing price when available, and otherwise falls back to that highest congestion value.
There was a problem hiding this comment.
Here, I would suggest to reuse the existing congestion tracker logic. Specifically, I would suggest to directly use the get_suggested_gas_price_for_objects() function which already does what you want. I would remove per_obj_min_clearing and per_obj_max_congestion and use something like:
let price = self.get_suggested_gas_price_for_objects(std::iter::once(*oid));
to store data into per_obj_required_in_cp.
Signed-off-by: salaheldinsoliman <[email protected]>
Signed-off-by: salaheldinsoliman <[email protected]>
Signed-off-by: salaheldinsoliman <[email protected]>
Signed-off-by: salaheldinsoliman <[email protected]>
Signed-off-by: salaheldinsoliman <[email protected]>
…col-research/nn-prediction-model-server
|
Changes summary to address the PR comments:
|
Description of change
This PR adds update to the neural network third party server via a function
inform_model.inform_modelis called at the end ofprocess_checkpoint_effects.Any logic regarding the update itself is placed in
model_updater.rs, that is not to clutter congestion_tracker with un-related logicLinks to any relevant issues
Be sure to reference any related issues by adding
fixes #(issue).How the change has been tested
Release Notes