-
Notifications
You must be signed in to change notification settings - Fork 42
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
[IO-1046][IO-1035] Team method upates and lazy loading #598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved - there's a couple of changes, but nothing I'd want to block this moving on
darwin/future/data_objects/team.py
Outdated
@@ -51,5 +55,32 @@ class Team(DefaultDarwin): | |||
# Data Validation | |||
_slug_validator = validator("slug", allow_reuse=True)(parse_name) | |||
|
|||
@staticmethod | |||
def from_client(client: Client, team_slug: Optional[str] = None) -> Team: | |||
"""Returns the team with the given slug""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do these with full docblocks, so they can be properly doc'd when it comes to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'll go back and write up docs for all the stuff I've added
darwin/future/data_objects/team.py
Outdated
members.append(TeamMember.parse_obj(item)) | ||
except Exception as e: | ||
errors.append(e) | ||
return (members, errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think the brackets are unnecessary, although totally stylistic, and your choice on this one.
result = self.filters[self.n] | ||
def __next__(self) -> R: | ||
if self.results is None: | ||
self.results = list(self.collect()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interaction here becomes
self.collect() -> MetaObject
Then MetaObject stores a list of R and implement iteration dunders
@@ -24,5 +24,6 @@ | |||
"black" | |||
], | |||
"python.testing.pytestEnabled": true, | |||
"python.linting.enabled": true | |||
"python.linting.enabled": true, | |||
"python.analysis.typeCheckingMode": "basic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have absolutely no idea what introduced this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments to definitely consider, but no outright demanded changes
@@ -24,5 +24,6 @@ | |||
"black" | |||
], | |||
"python.testing.pytestEnabled": true, | |||
"python.linting.enabled": true | |||
"python.linting.enabled": true, | |||
"python.analysis.typeCheckingMode": "basic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the looks of it, turns on pylance highlighting and syntax checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pylance being the built in checker that comes with vscode
from darwin.future.pydantic_base import DefaultDarwin | ||
|
||
T = TypeVar("T", bound=DefaultDarwin) | ||
T = TypeVar("T", bound=MetaBase) | ||
R = TypeVar("R", bound=DefaultDarwin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Genericing, 💯
return self + filter | ||
|
||
def __add__(self, filter: QueryFilter) -> Query[T]: | ||
def __add__(self, filter: QueryFilter) -> Query[T, R]: | ||
assert filter is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a note to replace these asserts with my assert_is
function which doesn't change in debug mode.
self.n += 1 | ||
return result | ||
else: | ||
raise StopIteration | ||
|
||
def __getitem__(self, index: int) -> R: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are our keys definitely always integers? __setitem__
and __getitem__
also take strings as native, but I can't immediately think whether we have any issue with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as in just implicitly converting a string to an int here, like query['6'] or would there be some other meaningful interaction with a string as an index that we'd want? Like query['name'] returning the dataset name for instance? I would worry that has too much overlap with like query.where('name=...') and we'd just be maintaining multiple sets of functionality that do the same job. IMO better to just have and maintain the one.
darwin/future/data_objects/team.py
Outdated
|
||
|
||
def get_team(client: Client, team_slug: Optional[str] = None) -> Team: | ||
"""Returns the team with the given slug""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment
"""Returns the team with the given slug""" | ||
if not team_slug: | ||
team_slug = client.config.default_team | ||
response = client.get(f"/teams/{team_slug}/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're starting to get a mix of functions that use the monad exceptions, return
response, and those that raise. Not something for now, but we should work out what our approach is for using one vs the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for me it comes down to when we get a single object back or a list of objects. If we're only returning a single 'block', ie something that's all related to each other and if one thing goes wrong it's all poisoned, then it makes sense to just return the object + raise if any issues, but if we have a function that returns a list of objects and they're all separate, then it makes sense to package up the good data and return it with the exceptions monad style.
@@ -27,3 +29,9 @@ def from_api_key(cls, api_key: str, datasets_dir: Optional[Path] = None) -> Meta | |||
if datasets_dir: | |||
config.datasets_dir = datasets_dir | |||
return cls(config) | |||
|
|||
@property | |||
def team(self) -> TeamMeta: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, pretty straightforward in the end.
|
||
R = TypeVar("R", bound=DefaultDarwin) | ||
|
||
class MetaBase(Generic[R]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was 50/50 whether to put this, but "metabase" is the name of a popular BI/Dashboarding tool. I wouldn't normally care, but we do actually use it elsewhere in the stack, so this might be worthy of renaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Meta Base object is definitely required for the generics, but don't mind what it's called.
assert_is(isinstance(dataset_id, int), "dataset_id must be an integer") | ||
|
||
dataset_deleted = remove_dataset(client, dataset_id) | ||
return dataset_deleted | ||
|
||
|
||
def __next__(self) -> Dataset: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers!
def collect(self, client: Client) -> List[TeamMember]: | ||
members, exceptions = get_team_members(client) | ||
def collect(self) -> TeamMembersMeta: | ||
members, exceptions = get_team_members(self.client) | ||
if exceptions: | ||
# TODO: print and or raise exceptions, tbd how we want to handle this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a think about this before we have many more of these.
Problem
Update darwin-py v2 to include Team Meta objects, lazy loading and querying.
Solution
Refactor of existing code from Core to Meta objects to enable lazy loading and query chaining.
Changes:
DatasetMeta(MetaBase[Dataset])
or genericallyclass T(MetaBase[R])
DatasetMeta.__next__()
.client.team[0].datasets[0]...
functionalityChangelog
Team + other meta objects now able to be chained and lazily loaded.