Create a new Buffer class that can flexibly combine the functions of current RingBuffer, Read, and Write. #771

tim-shea · 2023-08-06T04:29:32Z

Issue Number: #770

Objective of pull request: Create a new class Buffer that will dynamically create ports and vars as needed to allow the user to connect arbitrary inputs, outputs, and vars to pre-allocated numpy memory buffers. This will significantly simplify the the I/O package and make it easier to stimulate or record from networks.

Pull request checklist

Issue created that explains the change and why it's needed
Tests are part of the PR (for bug fixes / features)
Docs reviewed and added / updated if needed (for bug fixes / features)
PR conforms to Coding Conventions
PR applys BSD 3-clause or LGPL2.1+ Licenses to all code files
Lint (flakeheaven lint src/lava tests/) and (bandit -r src/lava/.) pass locally
Build tests (pytest) passes locally

Pull request type

Feature

Does this introduce a breaking change?

No

mathisrichter

I may be misunderstanding this. At least for output from a network, this seems to be overlapping with the Monitor Process. Would it make sense to add any output-related functionality to the Monitor?

I am somewhat confused by the API. It would expose Lava Vars to the application scope, independent of the Process that it was generated from. This may break up the concept of a Process too much. Maybe there is a way to add the Vars to the Buffer Process? I'm guessing you did not do this on purpose - let's discuss why. :)

mathisrichter · 2023-08-07T14:05:53Z

src/lava/proc/io/buffer.py

+        self.map_ref = []
+        self.index = 1000
+
+    def connect(self, other: Union[InPort, OutPort, Var],


It may be helpful to split this into a connect() and a connect_from() method, as we are doing it for ports. As it is now, I'm having trouble understanding the direction in which data flow will flow.

Yep, that would make sense. I'll make that change.

I don't love the names connect and connect_from but they're not awful and this would better differentiate the behavior of connecting to an InPort or Var (which will send the values from init) vs connecting from an OutPort or Var (in which case init will generally be overwritten).

mathisrichter · 2023-08-07T14:10:51Z

tests/lava/proc/io/test_buffer.py

+        lif = LIF(shape=(1,), du=1)
+        buffer = Buffer(length=10)
+        injector.out_port.connect(lif.a_in)
+        u = buffer.connect(lif.u)


So this creates a Var u but does not place it inside the buffer object? I was somehow not expecting that.

I can see how this is slightly confusing, because the use of dynamic port and var creation doesn't allow you to immediately see what's happening in PyBuffer, but the port and vars are added to the Buffer process and the corresponding PyPort and PyVars are added to the PyBuffer process model.

They can be accessed in the buffer.in_ports, buffer.out_ports, buffer.ref_ports, and buffer.vars collections, or they can be accessed directly with the use of a name as an attribute of the buffer. Unfortunately, I hit a small snag where I couldn't quite find a way to achieve user-specified names for those objects while also making it easy to intercept calls to the metaclass.getattr (necessary to convince the compiler to allow the process model to be built, but that's sort of a hack that could later be fixed in the compiler). Thus, the name of the var that will be used as a buffer is currently VarXXXX with the x's pulled from a incrementing sequence starting at 1000.

So, you could do:

buffer = Buffer() u = buffer.connect(some_other_proc.some_port) assert(u == buffer.Var1000)

I'll think a bit more on whether there's a way to let the user specify the name of the var, as I do find that more compelling than Var1000.

tim-shea · 2023-08-07T16:29:07Z

I may be misunderstanding this. At least for output from a network, this seems to be overlapping with the Monitor Process. Would it make sense to add any output-related functionality to the Monitor?

This is basically a refactor of the code from the current RingBuffer, Read, and Write processes. Read and Write definitely do overlap with Monitor, and it would be interesting to better understand how to eliminate that duplication. I started a refactor on Monitor a few months ago to turn it into a process with proper ports, but didn't make it far enough to commit anything. I'd be very interested to have a discussion to better understand Monitor architecture and try to reconcile these different I/O bits.

I am somewhat confused by the API. It would expose Lava Vars to the application scope, independent of the Process that it was generated from. This may break up the concept of a Process too much. Maybe there is a way to add the Vars to the Buffer Process? I'm guessing you did not do this on purpose - let's discuss why. :)

I don't know what you mean by 'application scope'. The goal here is not to alter the extent to which Vars are exposed, only to simplify the usage of the existing Read and Write processes which act like input and output buffers for Vars. Are you familiar with Read and the PyRead models in source.RingBuffer? This should replace those.

tim-shea · 2023-08-07T16:48:15Z

I am somewhat confused by the API. It would expose Lava Vars to the application scope, independent of the Process that it was generated from. This may break up the concept of a Process too much. Maybe there is a way to add the Vars to the Buffer Process? I'm guessing you did not do this on purpose - let's discuss why. :)

Ahh I think I figured out what you mean by exposing vars while responding to your other question. In the sense that my application code could end up with a Var called u, yes I see how that looks like it breaks down the encapsulation. The thing is, that reference to u is not the same u from the LIF process, it's a buffer into which u (the one from the LIF) will get written on each timestep. We return that var just to get around a bit naming ugliness (explained above) by giving the user a handy reference to their buffered data. Since the buffered version of u is always either input data specified by the user or output data which will be manipulated by the user, I don't see any issue with encapsulation there, although I suppose we could in theory not return the Var in the input case, since the initialization data goes in the method call (but then, what if I want to rerun with different data?) and return a Future (in the output case) to ensure the semantics of what you should do with "u" are clear.

gkarray

Very great addition to the I/O package, thanks @tim-shea !
Nothing much to say about the implementation itself, except that I like this trick of dynamic Port and Var creation :).
Otherwise, I added a few minor comments about syntax.

gkarray · 2023-08-18T10:39:05Z

src/lava/proc/plot/streaming.py

Would it make sense to add a small script/tutorial showing how to use the classes defined in this file ?
It doesn't have to be in this PR I guess, since it's not directly related to the Buffer concept you're introducing here.

gkarray · 2023-08-18T10:40:41Z

tests/lava/proc/io/test_buffer.py

+        injector = Injector(shape=(1,))
+        buffer = Buffer(length=10)
+        buffer.connect_from('v0', injector.out_port)
+        buffer.create_runtime(run_cfg=Loihi2SimCfg())


Is it necessary to explicitly call buffer.create_runtime before running ?
If so, please add a mention of this in the docstring of the Buffer class otherwise it might not be very straightforward for users to find out about it.

gkarray · 2023-08-18T10:47:46Z

tests/lava/proc/io/test_buffer.py

+from lava.proc.io.buffer import Buffer
+
+
+class TestBuffer(unittest.TestCase):


For consistency with how we test other Processes and ProcessModels in Lava, we might want to split tests into TestProcess and TestProcessModel. Honestly, I don't like that divide myself, and I prefer it the way you did it here...
But I think it is crucial for code quality to be consistent in how we test these things.
So if we decide to go this route and drop the divide between TestProcess and TestProcessModel in the future, we should at least write an issue about the need to refactor other Process tests.

gkarray · 2023-08-18T10:50:25Z

tests/lava/proc/io/test_buffer.py

+        self.assertEqual(buffer.v.shape, buffer_shape)
+        self.assertTrue(buffer.vInPort0 in buffer.in_ports)
+
+    def test_run_buffer(self):


For consistency with other test names, this should be something like test_run_single_input_buffer.

gkarray · 2023-08-18T10:50:42Z

tests/lava/proc/io/test_buffer.py

+        buffer.stop()
+        self.assertSequenceEqual(data, [0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
+
+    def test_buffer_with_2vars(self):


For consistency with other test names, this should be something like test_run_single_input_buffer_2_vars.

gkarray · 2023-08-18T10:50:55Z

tests/lava/proc/io/test_buffer.py

+        self.assertSequenceEqual(udata, [0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
+        self.assertSequenceEqual(vdata, [0, 1, 3, 6, 10, 0, 6, 0, 8, 0])
+
+    def test_multiple_buffers(self):


For consistency with other test names, this should be something like test_run_multiple_input_buffer.

gkarray · 2023-08-18T10:51:24Z

tests/lava/proc/io/test_buffer.py

+        self.assertSequenceEqual(udata, [0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
+        self.assertSequenceEqual(vdata, [0, 1, 3, 6, 10, 0, 6, 0, 8, 0])
+
+    def test_output_buffer(self):


For consistency with other test names, this should be something like test_run_single_output_buffer.
Oh, and don't forget the docstring for this test.

Signed-off-by: Marcus G K Williams <Marcus G K Williams 168222+mgkwill@users.noreply.github.com>

mgkwill · 2023-10-18T20:31:18Z

tests/lava/proc/io/test_extractor.py

        recv_data = [extractor.receive(), extractor.receive()]
+        if verbose:
+            print(f'{recv_data=}')
        extractor.stop()

        return np.array(recv_data)


test_receive_data_receive_not_empty_fifo and test_receive_data_receive_not_empty_accumulate appear to fail because self.runtime._is_running is not true at time of extractor.receive() and thus the check @tim-shea put in Extractor.can_receive() fails, thus the correct qsize is not used when calling internal _recieve_when*.

def can_receive(self) -> int: if self.runtime is not None and \ self.runtime._is_running: return self._pm_to_p_dst_port._queue.qsize() else: return 0

Run tests from venv using:

python -m unittest -v tests.lava.proc.io.test_extractor.TestPyLoihiExtractorModel.test_receive_data_receive_not_empty_fifo tests.lava.proc.io.test_extractor.TestPyLoihiExtractorModel.test_receive_data_receive_not_empty_accumulate

Signed-off-by: Marcus G K Williams <Marcus G K Williams 168222+mgkwill@users.noreply.github.com>

Initial version of Buffer.

050a68d

tim-shea linked an issue Aug 6, 2023 that may be closed by this pull request

Make it easier to use Buffers for input and output #770

Open

tim-shea self-assigned this Aug 6, 2023

tim-shea requested review from gkarray and bamsumit August 6, 2023 04:30

mathisrichter requested changes Aug 7, 2023

View reviewed changes

tim-shea added 2 commits August 14, 2023 13:33

Reworked Vars and Ports to allow natural naming.

8d04149

Added streaming Figure and Plots.

f4ea24a

gkarray requested changes Aug 18, 2023

View reviewed changes

mgkwill and others added 6 commits October 17, 2023 22:31

Merge branch 'main' into add_buffers

286f13b

Fix linting

5222288

Signed-off-by: Marcus G K Williams <Marcus G K Williams 168222+mgkwill@users.noreply.github.com>

Address test_buffer review

ff1c1ec

Signed-off-by: Marcus G K Williams <Marcus G K Williams 168222+mgkwill@users.noreply.github.com>

Fix codacy issues

30033f4

Signed-off-by: Marcus G K Williams <Marcus G K Williams 168222+mgkwill@users.noreply.github.com>

Fix codacy issues, add logging to tests

0a3f28d

Signed-off-by: Marcus G K Williams <Marcus G K Williams 168222+mgkwill@users.noreply.github.com>

Merge branch 'main' into add_buffers

bb893a9

mgkwill reviewed Oct 18, 2023

View reviewed changes

Marcus G K Williams added 2 commits October 18, 2023 13:37

Fix linting, formatting

7484c93

Signed-off-by: Marcus G K Williams <Marcus G K Williams 168222+mgkwill@users.noreply.github.com>

Merge branch 'add_buffers' of github.com:lava-nc/lava into add_buffers

815a1de

mgkwill force-pushed the main branch from 527dcfa to 5ed88f7 Compare November 15, 2023 04:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a new Buffer class that can flexibly combine the functions of current RingBuffer, Read, and Write. #771

Create a new Buffer class that can flexibly combine the functions of current RingBuffer, Read, and Write. #771

tim-shea commented Aug 6, 2023

mathisrichter left a comment

mathisrichter Aug 7, 2023

tim-shea Aug 7, 2023

mathisrichter Aug 7, 2023

tim-shea Aug 7, 2023

tim-shea commented Aug 7, 2023

tim-shea commented Aug 7, 2023

gkarray left a comment

gkarray Aug 18, 2023

gkarray Aug 18, 2023

gkarray Aug 18, 2023

gkarray Aug 18, 2023

gkarray Aug 18, 2023

gkarray Aug 18, 2023

gkarray Aug 18, 2023

mgkwill Oct 18, 2023 •

edited

		from lava.proc.io.buffer import Buffer


		class TestBuffer(unittest.TestCase):

Create a new Buffer class that can flexibly combine the functions of current RingBuffer, Read, and Write. #771

Are you sure you want to change the base?

Create a new Buffer class that can flexibly combine the functions of current RingBuffer, Read, and Write. #771

Conversation

tim-shea commented Aug 6, 2023

Pull request checklist

Pull request type

Does this introduce a breaking change?

mathisrichter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tim-shea commented Aug 7, 2023

tim-shea commented Aug 7, 2023

gkarray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgkwill Oct 18, 2023 • edited

Choose a reason for hiding this comment

mgkwill Oct 18, 2023 •

edited