Skip to content

BaseObject and rework of tags system, including dynamic tags#1134

Merged
fkiraly merged 47 commits intomainfrom
891-refactor
Jul 17, 2021
Merged

BaseObject and rework of tags system, including dynamic tags#1134
fkiraly merged 47 commits intomainfrom
891-refactor

Conversation

@fkiraly
Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly commented Jul 13, 2021

Rework of PR #1099 which is in turn a rework of PR #891 (by @RNKuhns), based on discussion recorded in #981 on tags.

Adds a BaseObject class which adds dynamic tag functionality. BaseObject contains BaseEstimator functionality sans fit state change related interfaces.
The new functionality replaces the inconsistent mixture of old interface points (all_tags, has_tag) in all downstream dependencies.

post-PR, tags work like this:

  • each class will have "class tags", stored in the content of the class variable _tags: dict. Individual class tags are considered to override each other as per normal nested inheritance. E.g., if a parent class and the class have different values for a class tag, the younger class' class tag value counts.
  • each object can, in addition, have dynamic tags, in the object variable _tags_dynamic: dict. These are expected to be initialized in the constructor, for instance in cases where behaviour described by tags depends on components or hyper-parameters. Dynamic tags are always considered to override the class tags - e.g., if the object's class or a parent class have a static tag of the same name, the dynamic tag's value overrides any class tag's value.

The new tag related methods make use of this inheritance logic are as follows.

class methods:

  • get_class_tags() collects all class tags and their values and returns them as a dict (keys = tag names, values = tag values). It overrides values precisely as described above, in the return dictionary. This replaces the old _all_tags.
  • get_class_tag(tag_name, tag_value_default=None) retrieves the value of an individual tag with name tag_name, and optionally allows to specify a default value if the tag is not found. The inheritance/override logic is the same as in get_class_tags.

object methods:

  • get_tags() collects all class tags and dynamic tags and their values and returns them as a dict (keys = tag names, values = tag values). It overrides values precisely as described above, in the return dictionary.
  • get_tag(tag_name, tag_value_default=None) retrieves the value of an individual tag with name tag_name, and optionally allows to specify a default value if the tag is not found. The inheritance/override logic is the same as in get_tags.
  • set_tags(**tag_dict) sets dynamic tags according to keys (=tag names) and values (= tag values) in the argument dictionary. It is expected to be used in constructors typically.
  • clone_tags(estimator, tag_set=None) is a shorthand version of set_tags, where it sets tags according to their get_tag values in estimator. The tags that are mirrored/copied in this way can be restricted by specifying a list of tag names in tag_set, otherwise all tags are copied over. The mirroring is by-reference and not by-value (if the type allows this), but tags are not expected to be mutated.

@fkiraly fkiraly requested review from aiwalter and mloning as code owners July 13, 2021 20:41
@fkiraly fkiraly requested a review from RNKuhns July 13, 2021 20:41
@fkiraly fkiraly added API design API design & software architecture implementing framework Implementing or improving framework for learning tasks, e.g., base class functionality labels Jul 13, 2021
@fkiraly fkiraly mentioned this pull request Jul 13, 2021
Copy link
Copy Markdown
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a first look at it, haven't reviewed all of it yet.

A few additional comments, perhaps for later implementation/PR:

  • in set_tags we should check that the passed tags are valid tags for the given estimator

@fkiraly fkiraly changed the title 891 refactor BaseObject and rework of tags system, including dynamic tags Jul 14, 2021
@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 14, 2021

in set_tags we should check that the passed tags are valid tags for the given estimator

I'd prefer not to, since this couples the BaseObject to the registry.
I also think we want to give the extender - or even the user - the opportunity to set their own tags easily, if they just work locally and hack around.

fkiraly and others added 2 commits July 14, 2021 12:48
Co-authored-by: Markus Löning <markus.loning@gmail.com>
Co-authored-by: Markus Löning <markus.loning@gmail.com>
@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 16, 2021

@mloning, can you please have a look? #980 is conditional on this, on which other stuff is conditional.
Would appreciate if you help unblocking the bottleneck.

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 16, 2021

(sorry for being pushy, but I like to believe it's for the best)

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 16, 2021

from discussion: value range checks should be done by object level tests
this will need additional convention in registry on items with discrete value space, e.g., which strings are admissible

Copy link
Copy Markdown
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing is still missing, everything else looks good, only a few minor comments.

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 16, 2021

Testing is still missing

No longer!

@fkiraly fkiraly requested a review from mloning July 16, 2021 19:22
Copy link
Copy Markdown
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few quick comments

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 17, 2021

tests run locally, builds were ok before merging doc related PR, so it's fine to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API design API design & software architecture implementing framework Implementing or improving framework for learning tasks, e.g., base class functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants