Skip to content

perf: sync sub index only when parent is synced#719

Merged
JoanFM merged 1 commit intomainfrom
perf-subindices-context-manager
Nov 8, 2022
Merged

perf: sync sub index only when parent is synced#719
JoanFM merged 1 commit intomainfrom
perf-subindices-context-manager

Conversation

@alaeddine-13
Copy link
Copy Markdown
Member

Currently, any elemental operation of a DA with subindices will attempt to sync the underlying sub indices even though the parent is not synced.
This PR tries to align the behavior and avoid the performance cost of syncing subindices at each operation.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 3, 2022

Codecov Report

Merging #719 (48346cd) into main (c38d82d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #719   +/-   ##
=======================================
  Coverage   88.28%   88.28%           
=======================================
  Files         134      134           
  Lines        6640     6641    +1     
=======================================
+ Hits         5862     5863    +1     
  Misses        778      778           
Flag Coverage Δ
docarray 88.28% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
docarray/array/storage/base/getsetdel.py 91.27% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

with subindex_da:
subindex_da.clear()
subindex_da.extend(docs)
subindex_da.clear()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not understand the problem, maybe a more clear description of the problem would clarify the scope

Copy link
Copy Markdown
Member

@JohannesMessner JohannesMessner Nov 8, 2022

Choose a reason for hiding this comment

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

The issue is the usage of the context manager for subindices. It is used "all the time", even if the main (parent) da does not use it, so subindex offset2ids are always synced, and there is no control over it.

This change undoes this, and instead ties the syncing of the subindices to the syncing of the parent, by making .sync() operate on all the subindices. I like it.

@JoanFM
Copy link
Copy Markdown
Member

JoanFM commented Nov 3, 2022

I would like to have a better description of this and understand the problem with a potential test

@JoanFM JoanFM merged commit 8f7c327 into main Nov 8, 2022
@JoanFM JoanFM deleted the perf-subindices-context-manager branch November 8, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants