Closed yoavkatz closed 3 weeks ago
To me operators should modify dictionaries in place, the reason is that that's how most people work with dictionaries. In cases where this creates confusion we should avoid it. Also I think we should have a separate issue for creating performance benchmark to unitxt we run on different pipelines, so we can keep track over performance across PRs.
The problem is when operators modify nested dictionary
instance: predictions : { "a" : 3, "b" :4} references : [{ "a" : 5, "b" :6}]
Then we have an operator Copy(field="predictions", to_field="orig_predictions"}
instance:
predictions : { "a" : 3, "b" :4}
orig_predictions = { "a" : 3, "b" :4} (same object as above)
references : [{ "a" : 5, "b" :6}]
Now we do Copy(field = "references/0/a" to, "predictions/a")
instance: predictions : { "a" : 5, "b" :4} orig_predictions = { "a" : 5, "b" :4} (same object as above < - notice how it was corrupted) references : [{ "a" : 5, "b" :6}]
How do you suggest we handle it?
So, until that metric that modifies predictions
is crucified, here is an attempt to reduce the damange:
https://github.com/IBM/unitxt/pull/991
Addressed at https://github.com/IBM/unitxt/pull/1087 An important lesson learned in the process is: https://github.com/IBM/unitxt/pull/1087#discussion_r1702268221
There is no bug in dict_utils. Dict_utils performs exactly what described in its docstring, and dict_utils has nothing to do with the case described here above.
If Unitxt wants to allow many metric evaluation on one model's output, there should be a careful save and restore of the stream post-model, in between metric evaluations (because some metrics do modify the stream loud and clear). This has nothing to do with dict_utils.
A solution to save and restore the stream in between metric evaluations, was presented in https://github.com/IBM/unitxt/pull/1119
However, this solution was not deployed.
On the contrary, deepcopies only spread out more and more in Unitxt, aiming at "ease debugging on our users".
So this aim costs performance.
Current dict_utils performs overwrite of dictionaries:
This caused problems because operators made changes to data of their source and it cause conflict with other operators. To workaround this, we now copy instances :
However this cause performance issue with large datasets (6x runtime increase) We should change the code such that new instances are returned with shallow copy - and only relevant atomic are copied.