Skip to content
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

adding comment creation support for domain layer & base repo #409

Merged
merged 5 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ repos:
exclude: (versioneer.py|_version.py)

- repo: https://github.com/timothycrosley/isort
rev: 5.12.0
rev: 5.13.2
hooks:
- id: isort

Expand Down
5 changes: 3 additions & 2 deletions rubicon_ml/domain/experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
from datetime import datetime
from typing import List, Optional

from rubicon_ml.domain.mixin import TagMixin
from rubicon_ml.domain.mixin import CommentMixin, TagMixin
from rubicon_ml.domain.utils import TrainingMetadata, uuid


@dataclass
class Experiment(TagMixin):
class Experiment(TagMixin, CommentMixin):
project_name: str

id: str = field(default_factory=uuid.uuid4)
Expand All @@ -20,4 +20,5 @@ class Experiment(TagMixin):
commit_hash: Optional[str] = None
training_metadata: Optional[TrainingMetadata] = None
tags: List[str] = field(default_factory=list)
comments: List[str] = field(default_factory=list)
created_at: datetime = field(default_factory=datetime.utcnow)
15 changes: 15 additions & 0 deletions rubicon_ml/domain/mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,18 @@ def remove_tags(self, tags: List[str]):
A list of string tags to remove from this domain model.
"""
self.tags = list(set(self.tags).difference(set(tags)))


class CommentMixin:
"""Adds comment support to a domain model."""

def add_comments(self, comments: List[str]):
"""
Add new comments to this model.

Parameters
----------
comments : List[str]
A list of string comments to add to the domain model.
"""
self.comments.extend(comments)
40 changes: 40 additions & 0 deletions rubicon_ml/repository/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1212,3 +1212,43 @@ def get_tags(self, project_name, experiment_id=None, entity_identifier=None, ent
sorted_tag_data = [json.loads(tag_data[p]) for _, p in sorted_tag_paths]

return sorted_tag_data

# ---------- Comments ----------

def _get_comment_metadata_root(
Copy link
Member

Choose a reason for hiding this comment

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

is there any difference between this and _get_tag_metadata_root? the metadata root is gonna be the same - you're just changing "tag" in the filename to "comment", which happens in the function you added below this one. if so, you can just have this function call _get_tag_metadata_root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's no real difference. I just thought it might be good form to add a comment specific one in case we wanted to differentiate between the two more in the future, but I think it's fine to remove now.

Copy link
Member

@ryanSoley ryanSoley Feb 14, 2024

Choose a reason for hiding this comment

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

yeah that definitely makes sense, and I could see the comments becoming a bit more complicated. if we just do something like

def _get_comment_metadata_root(args):
    # comments and tags are currently written to the same root with a different filename
    return self._get_tag_metadata_root(args)

we can have the best of both worlds - a new function available if it needs to change and reuse of the old one

self, project_name, experiment_id=None, entity_identifier=None, entity_type=None
):
"""Returns the directory to write comments to."""
# comments and tags are currently written to the same root with a different filename
return self._get_tag_metadata_root(
project_name, experiment_id, entity_identifier, entity_type
)

def add_comments(
self, project_name, comments, experiment_id=None, entity_identifier=None, entity_type=None
):
"""Persist comments to the configured filesystem.

Parameters
----------
project_name : str
The name of the project the object to comment
belongs to.
comments : list of str
The comment values to persist.
experiment_id : str, optional
The ID of the experiment to apply the comments
`comments` to.
entity_identifier : str, optional
The ID or name of the entity to apply the comments
`comments` to.
entity_type : str, optional
The name of the entity's type as returned by
`entity_cls.__class__.__name__`.
"""
comment_metadata_root = self._get_comment_metadata_root(
Copy link
Member

Choose a reason for hiding this comment

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

if my comment above is correct, you could just change this to _get_tag_metadata_root and leave a comment explaining that they're the same root path too. then just totally get rid of the above function. whichever is easier for you

project_name, experiment_id, entity_identifier, entity_type
)
comment_metadata_path = f"{comment_metadata_root}/comments_{domain.utils.uuid.uuid4()}.json"

self._persist_domain({"added_comments": comments}, comment_metadata_path)
14 changes: 11 additions & 3 deletions tests/unit/domain/test_mixin.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
from rubicon_ml.domain.mixin import TagMixin
from rubicon_ml.domain.mixin import CommentMixin, TagMixin


class Taggable(TagMixin):
def __init__(self, tags=[]):
class Taggable(TagMixin, CommentMixin):
def __init__(self, tags=[], comments=[]):
self.tags = tags
self.comments = comments


def test_add_tags():
Expand All @@ -25,3 +26,10 @@ def test_remove_tags():
taggable.remove_tags(["x"])

assert taggable.tags == ["y"]


def test_add_comments():
taggable = Taggable()
taggable.add_comments(["x"])

assert taggable.comments == ["x"]
22 changes: 22 additions & 0 deletions tests/unit/repository/test_base_repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -980,3 +980,25 @@ def test_get_tags_with_no_results(memory_repository):
)

assert tags == []


def test_add_comments(memory_repository):
repository = memory_repository
experiment = _create_experiment(repository)
repository.add_comments(
experiment.project_name,
["this is a comment"],
experiment_id=experiment.id,
entity_type=experiment.__class__.__name__,
)

comments_glob = f"{repository.root_dir}/{slugify(experiment.project_name)}/experiments/{experiment.id}/comments_*.json"
comments_files = repository.filesystem.glob(comments_glob)

assert len(comments_files) == 1

open_file = repository.filesystem.open(comments_files[0])
with open_file as f:
comments_json = json.load(f)

assert ["this is a comment"] == comments_json["added_comments"]
Loading