-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Slow performance due copying of instances #987
Comments
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: Then we have an operator Copy(field="predictions", to_field="orig_predictions"} instance: Now we do Copy(field = "references/0/a" to, "predictions/a") instance: How do you suggest we handle it? |
This is the point that wonders me, and without knowing, mentioned in our slack: please write the name of the metric that dares to change the predictions. |
Strangely enough, I manage to reproduce the above bug (that I thought should be a bug, indeed) only with 'bare' from unitxt.dict_utils import dict_get, dict_set
from unitxt.operators import Copy
from unitxt.stream import MultiStream
instance = {"predictions" : { "a" : 3, "b" :4}, "references" : [{ "a" : 5, "b" :6}]}
val = dict_get(instance,"predictions")
dict_set(instance, "predictions_orig", val)
instance["predictions"]["a"] = 10
assert(instance["predictions_orig"]["a"] == 10)
instance = {"predictions" : { "a" : 3, "b" :4}, "references" : [{ "a" : 5, "b" :6}]}
ms = MultiStream.from_iterables({"tmp" :[instance]})
copyoperator = Copy(field="predictions", to_field="predictions_orig")
ms = copyoperator(ms)
instance2 = list(ms["tmp"])[0]
assert(instance2["predictions_orig"] == instance2["predictions"])
instance2["predictions"]["a"] = 10
assert(instance2["predictions_orig"]["a"] == 3) |
So, until that metric that modifies |
in the branch suggested in the above PR: from unitxt.dict_utils import dict_get, dict_set
from unitxt.operators import Copy
from unitxt.stream import MultiStream
instance = {"predictions" : { "a" : 3, "b" :4}}
ms = MultiStream.from_iterables({"tmp" :[instance]})
copyoperator = Copy(field="predictions", to_field="predictions_orig")
ms = copyoperator(ms)
instance2 = list(ms["tmp"])[0]
assert(instance2["predictions_orig"] == instance2["predictions"])
instance2["predictions"]["a"] = 10
assert(instance2["predictions_orig"]["a"] == 10)
# and with use_deep_copy = true:
instance = {"predictions" : { "a" : 3, "b" :4}}
ms = MultiStream.from_iterables({"tmp" :[instance]})
copyoperator = Copy(field="predictions", to_field="predictions_orig", use_deep_copy=True)
ms = copyoperator(ms)
instance2 = list(ms["tmp"])[0]
assert(instance2["predictions_orig"] == instance2["predictions"])
instance2["predictions"]["a"] = 10
assert(instance2["predictions_orig"]["a"] == 3) |
Addressed at #1087 |
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.
The text was updated successfully, but these errors were encountered: