Implement queries mechanism for edges by asanin-epfl · Pull Request #129 · BlueBrain/snap

asanin-epfl · 2021-03-02T10:38:49Z

No description provided.

asanin-epfl · 2021-03-02T10:39:44Z

Probable more tests are required. @tomdele Should we rather merge tests of queries from nodes and edges into one place?

tomdele · 2021-03-02T11:42:57Z

+        unknown_props = properties - self.property_names
+        if raise_missing_prop and unknown_props:
+            raise BluepySnapError(f"Unknown edge properties: {unknown_props}")
+        data = self.get(None, properties - unknown_props)


I am quite scared this cannot be done. I allowed he None for the get because user is in full control but I am sure for huge circuit this will not be possible... Did you try ?

I am quite scared this cannot be done.

Should I understand this as "I am quite scared that this can be done"?

I allowed he None

Should I understand this as "I allowed the None"?

Did you try ?

No but I agree that this would highly likely fail/stuck on edges of Tb size with many properties.

In general this should work when there are only several properties within the query. All group attributes in edges files are int or floats. Lets assume float64 => 8 bytes. Lets say we have 1e9 (billion) edges in one population. One property then would take 8Gb. I doubt that edges of that size will be processed on a desktop computer. So, we have a high performant server hardware. Even for 10 properties it will take 80Gb of Ram which is not much for server hardware.

I don't want to introduce Dask until we are sure that libsonata will not support it for 100%.

I was not clear enough.

What I meant is:
Back in the days, the edges "queries" : get, afferent_nodes, efferent_nodes forced as entry a list of edge ids. Then I chose to allow the value None instead of a list of edge ids. And so I allowed : use all the edge_ids.
I did that because this is not hidden to the user and he clearly needs to call it:
Ex :

edge.get(None)

And the None is not a default value so a "bare" get() will raise.

Using the queries I am a bit scared because this is a bit more hidden and unclear for the user if he fills his memory.

I totally agree with the hardware but maybe we can do it a bit differently without dask.
What about doing the property queries by chunk ?

Something like :

res = [] ids = self.ids(None) critical_size = 60000000 # can be something computed using the dtypes of the properties for chunk in np.array_split(ids, len(ids) // critical_size): data = self.get(chunk, properties - unknown_props) res.extend(query.resolve_ids(data, self.name, queries))

?
doing so you reduce a lot the memory consumption because you don t load everything in memory at the same time and I guess with the change I made in #128 you should not have problems with the positional queries.

Great idea with chunking, I like it. Let see how it behaves. The current approach failed for 1.6Tb files (9.12 billion edges) with option --mem=100g and a single property. Around 74Gb is needed for single property in this case. The error was expected: Out Of Memory.

Thanks a lot for the check !
We will need to find a cleaver way of doing this so it can be usable ... I hope chunking can help.

--mem=0 worked for this 1.6Tb case but it took 1109.78255582 seconds.

yeah I did not expect insane perfs anyway.
Maybe we will be able to improve afterwards with multi processing.

ok, chunking is working and helps. It worked for 1.6Tb with --mem=200gb. Ran actually faster for 799.776455279999 seconds. --mem=100g is still not enough. In case when query is too broad and basically includes all of edges with multiple properties then chunking won't help.

tomdele · 2021-03-02T16:19:53Z

+        for chunk in np.array_split(ids, 1 + len(ids) // chunk_size):
+            data = self.get(chunk, properties - unknown_props)
+            res.extend(chunk[query.resolve_ids(data, self.name, queries)])
+        return res


We should use numpy array with IDS_DTYPE instead. I wrote the snippet with a list but I forgot we return arrays ...

btw wdyt about

res = [] ids = self.ids(None) chunk_size = int(1e8) for chunk in np.array_split(ids, 1 + len(ids) // chunk_size): data = self.get(chunk, properties - unknown_props) res.append(chunk[query.resolve_ids(data, self.name, queries)]) return np.concatenate(res, dtype=IDS_DTYPE)

This needs to be benchmarked. I did this housand times but keep forgetting about it. What s the best between what you did and concatenate at every chunk :

res = np.empty(dtype=IDS_DTYPE) ids = self.ids(None) chunk_size = int(1e8) for chunk in np.array_split(ids, 1 + len(ids) // chunk_size): data = self.get(chunk, properties - unknown_props) res.concatenate([res, chunk[query.resolve_ids(data, self.name, queries)]]) return res

(or something equivalent I did not run this piece of code just writing it live) ?

Did you manage to contain the memory consumption with the chunks?

Did you manage to contain the memory consumption with the chunks?

Well, it requires less memory for sure. 1.6Tb edges can be processed with --mem=200g and chunked version. The non-chunked version can process only with --mem=0 which is around 500g depending on the machine.

This needs to be benchmarked.

Not much difference in terms of memory. Used the below benchmark

pip install memory_profiler

from memory_profiler import profile import numpy as np np.random.seed(666) @profile def benchmark_append(): res = [] for i in range(10): start = np.random.randint(-10000, 10000) end = np.random.randint(100000, 1000000) res.append([i for i in range(start, end)]) return np.concatenate(res) @profile def benchmark_extend(): res = [] for i in range(10): start = np.random.randint(-10000, 10000) end = np.random.randint(100000, 1000000) res.extend([i for i in range(start, end)]) return np.array(res) if __name__ == '__main__': benchmark_append() benchmark_extend()

tomdele · 2021-03-03T09:37:24Z

+        for chunk in np.array_split(ids, 1 + len(ids) // chunk_size):
+            data = self.get(chunk, properties - unknown_props)
+            res.extend(chunk[query.resolve_ids(data, self.name, queries)])
+        return np.array(res, dtype=IDS_DTYPE)


do you think we could have better perfs using smaller but more chunks and using multiprocessing (not dask just multiprocess)?

I don't know. The problem here is the layer of libsonata between us and HDF5. If we go directly via h5py then such optimization would work. Otherwise, I am not sure.
Another thought is that such optimization can be a bottleneck. Bluepysnap is tool to access circuits. It will be used a lot in projects that already are parallelized. It is highly likely that there will be no available proc cores for multiprocessing. So all this setting up of multiprocessing will be unnecessary and make things slower in the end.

Hum good points ... Ok we can keep it like this.

It is not necessary to merge it. We can abandon. I've implemented rather for history so we can have this discussion. If you still have some doubts, lets postpone or even abandon.

No I think this is good to have this feature and we can improve it once it is here already.

My only concern is to crash people pc when they are doing something they don t intend to. So the best would be to have something that can evaluate the size of the returned property dataframe in the function and compare it to the size of the user memory. So we can throw and clean memory before the ram + swap are full.

tomdele · 2021-03-03T10:57:26Z

+            raise BluepySnapError(f"Unknown edge properties: {unknown_props}")
+        res = []
+        ids = self.ids(None)
+        chunk_size = int(1e8)


We can keep it like this for the moment but do you think we can keep it close to ~10Gb of memory usage in the future with a cleaver use of the chunk_size ?

Do you mean to detect the amount of available RAM by cleaver usage?

yes I was writing it here :
#129 (comment)

What do you think ?

I don t have a super strong opinion. I just hate when my pc freeze when it was not clearly expected.

Please confirm

detect available RAM memory

detect size of edges file

raise an exception if RAM is too low

otherwise calculate chunk size, and start processing

Thanks for the check.
I must say I am not sure what we can do to avoid filling memory at the moment. So let s merge it like this and we will see.
Maybe reducing the chunk size to 1e7 ?

ok but I want to emphasize again that if there is not enough memory => we fail at libsonata usage before reaching this code. Basically when we in def _edge_ids_by_filter, we already have enough memory.

Maybe reducing the chunk size to 1e7 ?

1e8 of bytes is 100Mb. Are you sure to reduce it to 10Mb?

ahhh ok. Sorry that s all good then !

Thank you so much. It was very helpful and useful.

tomdele · 2021-03-04T09:32:23Z

Thanks a lot for this is really nice !

Implement queries mechanism for edges

f80e1a5

tomdele reviewed Mar 2, 2021

View reviewed changes

chunking for edge queries

51bf464

tomdele reviewed Mar 2, 2021

View reviewed changes

return np.array with IDS_DTYPE

d0770c1

tomdele reviewed Mar 3, 2021

View reviewed changes

tomdele approved these changes Mar 3, 2021

View reviewed changes

asanin-epfl merged commit 36b522e into master Mar 4, 2021

asanin-epfl deleted the query-edges branch March 4, 2021 09:22

Uh oh!

Conversation

asanin-epfl commented Mar 2, 2021

Uh oh!

asanin-epfl commented Mar 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdele Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asanin-epfl Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asanin-epfl Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdele Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asanin-epfl Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdele Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdele commented Mar 4, 2021

Uh oh!

Reviewers

Assignees

tomdele Mar 2, 2021 •

edited

Loading

asanin-epfl Mar 2, 2021 •

edited

Loading

asanin-epfl Mar 3, 2021 •

edited

Loading

tomdele Mar 3, 2021 •

edited

Loading

asanin-epfl Mar 4, 2021 •

edited

Loading

tomdele Mar 4, 2021 •

edited

Loading