Support dict in set tags

[yarnabrina]

Closes sktime/sktime#5960

[fkiraly]

Hm, I second the idea, but this will break working code, will it not? Where previously people have used explicit kwargs, it will now lead to an exception unless the first kwarg is a dict.

Can we do something that is completely deprecation safe?

[yarnabrina]

Why will it break the code? I explicitly test that passing just kwargs will continue to work.

    dummy_object_instance.set_tags(bar=84)
    assert dummy_object_instance.get_tag("bar") == 84

[fkiraly]

It would break the code est.set_tags(tags_to_set="bar"), no? Perhaps that is too rare though. Still, strictly speaking the change falls in the "interface breaking" class, which indicates there might be overlooked long-term consequences.

How about we use args, and continue with the intended new behaviour, all args assumed dict?

[fkiraly]

so, for all arg in args assert dict, and kwargs.update(arg)?

[yarnabrina]

I'm not able to follow you. Can you please explain which of these steps is the issue?

[yarnabrina]

I ran all tests locally (fixed a typo in test) and all pass.

❯ pytest skbase/tests/test_base.py -k "test_set_tags_by_dict_and_kwargs" -v
===================================== test session starts =====================================
platform linux -- Python 3.10.12, pytest-7.4.0, pluggy-1.0.0 -- /home/anirban/conda-environments/sktime/bin/python
cachedir: .pytest_cache
rootdir: /home/anirban/skbase
configfile: pyproject.toml
plugins: dash-2.11.1
collected 53 items / 52 deselected / 1 selected                                               

skbase/tests/test_base.py::test_set_tags_by_dict_and_kwargs PASSED                      [100%]

============================== 1 passed, 52 deselected in 1.13s ===============================

[fkiraly]

The issue is, this is in-principle an interface breaking change. Adding a keyword arg at the start always is. In this case, the change is equivalent to doing

if "tags_to_set" in kwargs:
    kwargs.update(kwargs.pop("tags_to_set"))

My comment: if you want to do obj.set_tags({"foo": "bar"}). why not use args instead? That is,

for args in args:
    kwargs.update(arg)

[yarnabrina]

The issue is, this is in-principle an interface breaking change.

I'm very sorry, but this is the part that I still don't understand. May be (must, not may) I'm missing something obvious, but can you please explain?

if you want to do obj.set_tags({"foo": "bar"}). why not use args instead

This is also something I don't understand. *args results in a tuple, not a dict. How shall I take key and value?

If you mean use *args, pass a dict, and loop over args[0], in my opinion that's bad as we are ignoring rest of the variable positional arguments.

[fkiraly]

May be (must, not may) I'm missing something obvious, but can you please explain?

An interface breaking change is a change where previously working code breaks after the change.

Examples of hanges that are always interface breaking:

• adding a parameter to a kwarg-less function, not at the end
• adding a parameter to a function with kwargs, and later not merging it into kwargs as key/value pair (or equivalent)
• changing the sequence of parameters of a function

It suffices to show an example of breaking code to prove that a change is interface breaking - or check whether it is a subcase of above examples (if you believe these are interface breaking).

An example for this change is est.set_tags(tags_to_set="bar").

If you mean use *args, pass a dict, and loop over args[0], in my opinion that's bad as we are ignoring rest of the variable positional arguments.

Just loop over all args[i] and allow multiple dictionaries?

Like this:

for args in args:
    kwargs.update(arg)

Why is this bad, can you explain?

[yarnabrina]

An example for this change is est.set_tags(tags_to_set="bar").

Is your concern only for the EXTREMELY specific case of tag name being "tags_to_set" that matches the newly introduced dictionary argument? If that's the case, this is probably too rare. When you add new tags in sktime, how do we know those tags are already not being used by users in their customised estimators with different type and different supported values, will you consider that as breaking as well?

For now, I've tried to handle with if-else. Please take a look. It's still breaking if you consider even more specific case of tags_to_set and None being the desired tag name and value respectively.

---

Why is this bad, can you explain?

When I said that, my impression was you are expecting users to pass only one dictionary as part of args and ignore everything that follows before keyword arguments start. In that case, I felt it is bad as function signature is suggesting arbitrary number of parameters but actually that's not being used. Based on your reply, I don't think this is what you meant. I'll make a comment on my updated understanding of your suggestion.

---

Just loop over all args[i] and allow multiple dictionaries?

Currently we have to use est.set_tags(**{"tag-name": "tag-value"}), and since this is must for (almost) all sktime tags (as they have hyphen) and ** seems unnecessary, I wanted to support est.set_tags({"tag-name": "tag-value"}). With concern of breaking change, I allowed to support est.set_tags(**{"tag-name": "tag-value"}, another_tag_name=another_tag_value).

If I understand your suggestion, you are expecting developers to pass est.set_tags({"tag-name": "tag-value"}, {"another_tag_name": another_tag_value}). I don't think this improves developer experience. Plus here I also need to validate every single element in args is a dictionary.

[fkiraly]

If I understand your suggestion, you are expecting developers to pass est.set_tags({"tag-name": "tag-value"}, {"another_tag_name": another_tag_value}).

Not at all! This is just an option, your desired est.set_tags({"tag-name": "tag-value", "another_tag_name": another_tag_value}) would still work.

Plus here I also need to validate every single element in args is a dictionary.

I think the code is much simpler stlil?

for arg in args:
    assert isinstance(arg, dict), "Oh no!"
    kwargs.update(arg)

[fkiraly]

(no particularly strong opinions though, just making sure we
have discussed this)