not_exist_ok for dict_set only allows to add a non existing key to a dictionary, or non existing entry to a list. Not a whole nested structure implied by the query #1402

dafnapension · 2024-11-29T16:39:32Z

closes: #1400

Can still set a whole nested structure, but as the value being set, not through processing the query by dict_set.

dafnapension · 2024-11-29T21:51:52Z

Hi @yoavkatz , I think the needed patches are rather ugly... and I hardly managed to cover all (with axes and pry-bars..; translating from Shai K Ophir, min 4:32 here). Users will have to employ similar measures in the future, or perhaps change their existing code right now, if we pull the rag under their legs (the rag that allowed to dict_set a whole construct through the query; although that rag looked odd to you).
I recommend to return back home..

yoavkatz · 2024-12-01T07:37:07Z

prepare/cards/mmmu.py

@@ -47,6 +47,7 @@
        ),
        preprocess_steps=[
            RenameSplits(mapper={"dev": "train", "validation": "test"}),
+            SetEmptyDictIfDoesNotExist(field="media"),


I think the 'media' field is used by generic code, so it should be added by the code and not the cards.

See for example:

unitxt/src/unitxt/task.py

Line 268 in fc91a7e

"media": instance.get("media", {}),

Hence, this field should be added

https://github.com/IBM/unitxt/blob/reduce-error-message-clutter/src/unitxt/standard.py#L297

yoavkatz · 2024-12-01T07:52:15Z

src/unitxt/operators.py

@@ -277,6 +278,20 @@ def process(
        return instance


+class SetEmptyDictIfDoesNotExist(InstanceOperator):


The behavior of this is method is more than expected by the name. It not only creates an empty dict if it does not exist, but also transform the field from string to dict if string is a json. I think we should avoid such complex behavior, which is more indicative of a bug somewhere else.

For example, on Friday I found a bug in post processing pipeline:

(Still not in main)

Hi @yoavkatz , If this helps, I found what triggered my need to add this json.loads to SetEmptyDictIfDoesNotExist . Again, just for patching with גרזנים ולומיק the need to manually generate container elements (lists and dicts, thus far I wrote a לומיק just for dicts, but will need to add lists too), to not surprise the user of dict_set:

This method, shamelessly makes task_data into a string:

def serialize_instance_fields(self, instance, task_data): if settings.task_data_as_text: instance["task_data"] = json.dumps(task_data) if not isinstance(instance["source"], str): instance["source"] = json.dumps(instance["source"]) return instance

dafnapension · 2024-12-02T19:29:02Z

Hi @yoavkatz , I suspect that the new SetEmptyDictIfDoesNotExist that I added, and the about to be added SetEmptyListIfDoesNotExist and the enforcement to use them before each and every existing Set and more over: each and every existing Operator that writes anything to any to_field, will drive our users crazy. Let alone the runtime it will consume.
I suggest to consider any of the following solutions:
(1) extend the possible values of not_exist_ok to : No, at_most_last_component, any_missing_component,
and throw exception accordingly (my recommendation)
(2) only allow at_most_last_component (that you feel is the decent, expected behavior), and then once an operator and an input instance like those that started the current saga will meet:
applying operator=Copy(field="source", to_field="task_data/source") on input instance
{"source": "hello", "task_data" :3} will throw a Warning, saying something like:
operator Copy employed dict_set in order to set value "hello" to an element (subfield) "task_data/source" that does not exist in the input instance, nor its implied containing subfield of type dict "task_data". (which exists, but is not a dictionary). dict_set is not allowed to generate into the instance elements implied by the query, other than the element implied by the last component.
dict_set was invoked instead with the following query and value: dict_set("task_data", {"source" : "hello"}) which sets a value, of type dict, to an existing element, "task_data", replacing its old value. This seems to achieve the requested result, and still comply with the rule that dict_set changes at most the element identified by the last component of the query.

Other than these explanation and apologies, the computed results will remain the same, users will not have to change their existing code, and/or write tedious future invocations of dict_set s.

yoavkatz · 2024-12-03T16:37:44Z

Hi @yoavkatz , I suspect that the new SetEmptyDictIfDoesNotExist that I added, and the about to be added SetEmptyListIfDoesNotExist and the enforcement to use them before each and every existing Set and more over: each and every existing Operator that writes anything to any to_field, will drive our users crazy. Let alone the runtime it will consume. I suggest to consider any of the following solutions: (1) extend the possible values of not_exist_ok to : No, at_most_last_component, any_missing_component, and throw exception accordingly (my recommendation) (2) only allow at_most_last_component (that you feel is the decent, expected behavior), and then once an operator and an input instance like those that started the current saga will meet: applying operator=Copy(field="source", to_field="task_data/source") on input instance {"source": "hello", "task_data" :3} will throw a Warning, saying something like: operator Copy employed dict_set in order to set value "hello" to an element (subfield) "task_data/source" that does not exist in the input instance, nor its implied containing subfield of type dict "task_data". (which exists, but is not a dictionary). dict_set is not allowed to generate into the instance elements implied by the query, other than the element implied by the last component. dict_set was invoked instead with the following query and value: dict_set("task_data", {"source" : "hello"}) which sets a value, of type dict, to an existing element, "task_data", replacing its old value. This seems to achieve the requested result, and still comply with the rule that dict_set changes at most the element identified by the last component of the query.

Other than these explanation and apologies, the computed results will remain the same, users will not have to change their existing code, and/or write tedious future invocations of dict_set s.

Hi. I guess what I'm missing is how prevalent is this problem and wherher its actually a problem caused by another issue.. I means media and task data are generic fields that should always be there and added by generic cide. If not, we need to understand why.

In general, we need to strongly avoid complex flags, that increase the learning curve of the library.

dafnapension · 2024-12-04T08:10:56Z

I agree (although I like flags..). Can this issue hint to what you are looking for? I bumped into it while needing to push this LoadJson into the patch.
You forgot to mention recipe_metadata that is assumed to exist, at least by Operator ApplyTempate.

…dictionary, or non existing entry to a list. Not a nested structure as implied by the query Signed-off-by: dafnapension <[email protected]>

…w fails if dict_set only adds to an existing dictionary Signed-off-by: dafnapension <[email protected]>

Signed-off-by: dafnapension <[email protected]>

…t ensure task_data exists, because default value is used Signed-off-by: dafnapension <[email protected]>

…dexing into a list element Signed-off-by: dafnapension <[email protected]>

Signed-off-by: dafnapension <[email protected]>

…dictionary, or non existing entry to a list. Not a nested structure as implied by the query Signed-off-by: dafnapension <[email protected]>

Signed-off-by: dafnapension <[email protected]>

…mpatibility Signed-off-by: dafnapension <[email protected]>

Signed-off-by: dafnapension <[email protected]>

dafnapension · 2024-12-15T16:48:54Z

Too much existing code trust dict_set to fill in all missing component, and not just the last component

dafnapension force-pushed the simpler_dict_utils branch from acdc7ac to 17ce7d3 Compare November 29, 2024 19:47

yoavkatz reviewed Dec 1, 2024

View reviewed changes

dafnapension force-pushed the simpler_dict_utils branch 3 times, most recently from 5019948 to 3c1ab49 Compare December 2, 2024 18:53

dafnapension force-pushed the simpler_dict_utils branch from 3c1ab49 to e5e8be4 Compare December 3, 2024 15:37

dafnapension added 11 commits December 4, 2024 19:45

not_exist_ok for dict_set only allows to add a non existing key to a …

c2dd9ea

…dictionary, or non existing entry to a list. Not a nested structure as implied by the query Signed-off-by: dafnapension <[email protected]>

prepend a special solution for set into 'recipe_metadata/xxx' that no…

05e212f

…w fails if dict_set only adds to an existing dictionary Signed-off-by: dafnapension <[email protected]>

yet another patch to prepare for setting into a query path

df939d4

Signed-off-by: dafnapension <[email protected]>

patch also in prepare files, where the from_field = task_data does no…

6f340f3

…t ensure task_data exists, because default value is used Signed-off-by: dafnapension <[email protected]>

for backward compatibility, index into a string element is OK like in…

6e3b366

…dexing into a list element Signed-off-by: dafnapension <[email protected]>

yet another point in prepare/cards that needed a patch

e3b96cd

Signed-off-by: dafnapension <[email protected]>

polished and unified error msgs, and updated expecetd error texts

2a8b1e9

Signed-off-by: dafnapension <[email protected]>

not_exist_ok for dict_set only allows to add a non existing key to a …

344d090

…dictionary, or non existing entry to a list. Not a nested structure as implied by the query Signed-off-by: dafnapension <[email protected]>

adapt to the new error texts

6549873

Signed-off-by: dafnapension <[email protected]>

returned a test that depended on indexing into a str, for backward co…

32f6f1a

…mpatibility Signed-off-by: dafnapension <[email protected]>

some docstrings

1c8546f

Signed-off-by: dafnapension <[email protected]>

dafnapension force-pushed the simpler_dict_utils branch from e5e8be4 to 1c8546f Compare December 4, 2024 17:45

dafnapension marked this pull request as draft December 13, 2024 14:20

dafnapension closed this Dec 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

not_exist_ok for dict_set only allows to add a non existing key to a dictionary, or non existing entry to a list. Not a whole nested structure implied by the query #1402

not_exist_ok for dict_set only allows to add a non existing key to a dictionary, or non existing entry to a list. Not a whole nested structure implied by the query #1402

dafnapension commented Nov 29, 2024

dafnapension commented Nov 29, 2024 •

edited

Loading

yoavkatz Dec 1, 2024 •

edited

Loading

yoavkatz Dec 1, 2024

dafnapension Dec 2, 2024 •

edited

Loading

dafnapension commented Dec 2, 2024 •

edited

Loading

yoavkatz commented Dec 3, 2024

dafnapension commented Dec 4, 2024 •

edited

Loading

dafnapension commented Dec 15, 2024

		@@ -277,6 +278,20 @@ def process(
		return instance


		class SetEmptyDictIfDoesNotExist(InstanceOperator):

not_exist_ok for dict_set only allows to add a non existing key to a dictionary, or non existing entry to a list. Not a whole nested structure implied by the query #1402

not_exist_ok for dict_set only allows to add a non existing key to a dictionary, or non existing entry to a list. Not a whole nested structure implied by the query #1402

Conversation

dafnapension commented Nov 29, 2024

dafnapension commented Nov 29, 2024 • edited Loading

yoavkatz Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

yoavkatz Dec 1, 2024

Choose a reason for hiding this comment

dafnapension Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

dafnapension commented Dec 2, 2024 • edited Loading

yoavkatz commented Dec 3, 2024

dafnapension commented Dec 4, 2024 • edited Loading

dafnapension commented Dec 15, 2024

dafnapension commented Nov 29, 2024 •

edited

Loading

yoavkatz Dec 1, 2024 •

edited

Loading

dafnapension Dec 2, 2024 •

edited

Loading

dafnapension commented Dec 2, 2024 •

edited

Loading

dafnapension commented Dec 4, 2024 •

edited

Loading