-
Notifications
You must be signed in to change notification settings - Fork 9
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
Partial context updates #93
base: dev
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
chatsky/context_storages/sql.py
Outdated
async with self.engine.begin() as conn: | ||
await conn.execute(update_stmt) |
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.
It'd be better to define a method for saving context in DBContextStorage (i.e. ContextStorage method that accepts Context and saves it).
That way we can send all changes in one commit because there'd be access to engine from ContextStorage.
Right now we basically do
COMMIT main_info
COMMIT labels_added_modified
COMMIT labels_removed
COMMIT requests_added_modified
COMMIT requests_removed
Instead of a single
COMMIT main_info, labels_added_modified, labels_removed, requests_added_modified, requests_removed
Why this might be relevant: in case one of the methods fails we don't want others to succeed (this will lead to a broken context). And we can also have one place where we can process the errors.
I think we should improve db context storage api as a patch after release.
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.
Yes, we could. However, I am not sure that even for SQL databases executing many operations with one "connection" would protect us from breaking context structure (i.e. I don't think that writes will become atomic), however we can try.
del self._items[key] | ||
|
||
def __iter__(self) -> Sequence[int]: | ||
return iter(self.keys() if self._storage is not None else self._items.keys()) |
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.
Why not yield from self.keys()
?
I.e.:
- Why not use
self.keys()
regardless of the_storage
? - Why not use
yield from
?
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 just forgot yield from
construction exists at all, it is not widely used...
node = self.framework_data.current_node | ||
if node is None: | ||
raise ContextError("Current node is not set.") | ||
return node | ||
|
||
async def turns(self, key: Union[int, slice]) -> Iterable[Tuple[AbsoluteNodeLabel, Message, Message]]: |
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.
Make turns a property that returns an instance of a Turns
class (needs to be defined) with async __getitem__
.
So that the usage of turns
is await ctx.turns[1:3]
instead of await ctx.turns(slice(1, 3))
.
Also, make it so that turns[1]
is a single tuple while turns[1:2]
is an iterable of tuples.
if isinstance(key, slice): | ||
return [self._items[k] for k in self.keys()[key]] | ||
else: | ||
return self._items[key] |
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 I don't like about the way slices are processed now is that single key refers to a specific turn while a slice refers to a range of available turns.
So if there are only turns 1, 3, 5 available in ctx_dict, ctx_dict[1:2] == ctx_dict[3], ctx_dict[5]
, which is very confusing.
I think that there's little use case to getting ctx_dict items by their position in the keys.
So my proposal is:
Allow only
ctx_dict[non-neg-int]
-- return element on turnnon-neg-int
(turn number is a non-negative integer);ctx_dict[non-neg-int1:non-neg-int2:non-neg-int3]
-- return elements starting from turnnon-neg-int1
up tonon-neg-int2
with a step ofnon-neg-int3
.
(just in case: this proposal is for all the methods)
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 agree
""" | ||
|
||
try: | ||
return await self[key] |
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.
This will always fail if key
is Iterable
.
This should be rewritten as
await self.load_items(keys if isinstance(keys, Iterable) else [keys])
if isinstance(key, Iterable):
return [self._items.get(k, default) for k in key]
else:
return self._items.get(k, default)
Tests for using get with iterable key should be added.
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.
Fair enough
Description
Context storages are updated partially now instead of reading and writing whole data at once.
Checklist
UpdateScheme
fromBaseModel
clear
method.