Skip to content
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 bridge Processes #686

Merged
merged 39 commits into from
Jul 10, 2023
Merged

IO bridge Processes #686

merged 39 commits into from
Jul 10, 2023

Conversation

gkarray
Copy link
Contributor

@gkarray gkarray commented May 12, 2023

Issue Number: #687

Objective of pull request: Addition of IO bridge (Python only) Processes for getting input to/output from Lava: Injector and Extractor.

Pull request checklist

Your PR fulfills the following requirements:

  • 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

Please check your PR type:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation changes
  • Other (please describe):

What is the current behavior?

  • At the moment, the only ways to get data into Lava, from a Python application, is to go through the Dataloader or RingBuffer Processes, or the set() method on Process Var.
    • The Dataloader Process keeps data in disk, and only loads portions of it step-by-step.
    • The RingBuffer Process loads all data to memory from the start.
    • The set() method requires running Lava workloads one time step at a time, and calling it between each run() call.
  • Similarly, the only ways to get data out of Lava, to use in a Python application, is to go through the RingBuffer Process, or the get() method on Process Var.
  • The RingBuffer Process buffers data in memory, and one has to call get() at the end of the run on its data Var to get the data out.
  • The get() method requires running Lava workloads one time step at a time, and calling it between each run() call.

What is the new behavior?

  • With the Injector Process on the input side, users would be able to seamlessly integrate Lava workloads (running in non-blocking mode (!!)) into broader applications. These Lava workloads would be able to get dynamically generated input data from Python applications, without having to pause and re-run every time step.
    • "Dynamically generated input data" means: Without having to pre-load the data from the start (in contrast with the RingBuffer option).
    • "From Python applications" means: Not loading from disk (in contrast with the Dataloader option).
    • "Without having to pause and re-run every time step" means: Lava workload won't have to run one time step at a time (in contrast with the set() option).
  • With the Extractor Process on the output side, users would be able to seamlessly integrate Lava workloads into broader applications. These broader applications would be able to get real-time output data from Lava workloads, without having to pause and re-run every time step.
    • "Real-time output data from Lava workloads" means: Without having to wait for a Lava run to finish to get data out (in contrast with the RingBuffer option).
    • "Without having to pause and re-run every time step" means: Lava workload won't have to run one time step at a time (in contrast with the get() option).

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

TODO:

  • Add variants with VEC_SPARSE.
  • Add (or change to) AsyncProcessModels, implementing the AsyncProtocol.

@gkarray gkarray self-assigned this May 12, 2023
@gkarray gkarray requested a review from SveaMeyer13 May 12, 2023 10:53
@gkarray gkarray changed the title First prototype of IO bridge Processes IO bridge Processes May 12, 2023
@tim-shea tim-shea linked an issue May 14, 2023 that may be closed by this pull request
Copy link
Contributor

@tim-shea tim-shea left a comment

Choose a reason for hiding this comment

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

Great work on this PR @gkarray

At a high-level, I think it's an important feature to enable, but I'm not convinced this method is the right way to go about it. This pipe-from-the-parent-process will introduce temporal and behavioral coupling between lava and non-lava code, but may not behave as a user expects if their execution is not correctly synchronized.

At minimum, it would be good to see all of the following behaviors tested and clearly documented to users:

  • Run the calling code and lava models for many timesteps
  • Run the calling code and lava models for different number of timesteps (e.g. send_data 100 times while RunSteps=50 and vice versa)
  • Start calling send_data before calling proc.run
  • Send and receive non-trivial data structure
  • Test the edge cases for pipe filling up and emptying out (i.e. if the calling code for InputBridge.send_data runs signficantly faster than the lava code, confirm whether it will eventually block or raise; vice versa for calling code for OutputBridge.recv_data)

@tim-shea tim-shea added this to the Lava v0.8 milestone May 15, 2023
@tim-shea
Copy link
Contributor

Since I don't see any replies but I do see a bunch of new commits, I'm not sure what you think of my comments above, but here's a more concrete proposal for naming:

input_bridge.py > input_synchronizer.py
InputBridge > InputSynchronizer
AbstractPyLoihiInputBridgeProcessModel > PyInputSynchronizerModel
PyLoihiFloatingPointInputBridgeProcessModel > PyInputSynchronizerModelFloat
PyLoihiFixedPointInputBridgeProcessModel > PyInputSynchronizerModelFixed

output_bridge.py > output_synchronizer.py
OutputBridge > OutputSynchronizer
AbstractPyLoihiOutputBridgeProcessModel > PyOutputSynchronizerModel
etc...

You really don't need async protocol models for the synchronizer processes, because the way they're written makes them useful specifically for synchronizing your code to your synchronous Loihi model, hence the renames to describe their actual function.

See also Lif models.py, Dense models.py, etc, where the models start with Py, not PyLoihi, and include Model, but not ProcessModel.

Continuing:
in_bridge.py > async_injector.py
AsyncInputBridge > AsyncInjector
AsyncProcessDenseModel > AsyncInjectorModelFloat

Note that the process name should describe the behavior of all models, in this case Async should not refer to Async protocol vs Loihi protocol, but to the behavior in which the input is injected asynchronously with respect to the updates of the connected port. Basically, this process should allow me to sporadically send data or recv data without caring whether I send or recv the correct number of times, and without ever blocking my calling code or the connected port.

out_bridge.py > async_extractor.py
AsyncOutputBridge > AsyncExtractor
AsyncProcessModel > AsyncExtractorModelFloat

@PhilippPlank PhilippPlank marked this pull request as ready for review June 13, 2023 09:05
Copy link
Contributor

@tim-shea tim-shea left a comment

Choose a reason for hiding this comment

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

Very nice! Great cleanups, this is a clear, simple, and super useful little addition to the core Lava API.

Only top level suggestion to add is to drop the "bridge" in module paths, and just locate these three modules in io.

src/lava/proc/io/bridge/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/injector.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/injector.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mathisrichter mathisrichter left a comment

Choose a reason for hiding this comment

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

Looks good overall - some minor changes and naming suggestions.
We did this in a live review, so my comments are a bit short; more reminders for @gkarray .

src/lava/proc/io/input_bridge.py Outdated Show resolved Hide resolved
src/lava/proc/io/bridge/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/extractor.py Show resolved Hide resolved
src/lava/proc/io/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/injector.py Show resolved Hide resolved
src/lava/proc/io/utils.py Show resolved Hide resolved
tests/lava/proc/io/test_injector.py Outdated Show resolved Hide resolved
tests/lava/proc/io/test_injector.py Outdated Show resolved Hide resolved
src/lava/proc/io/extractor.py Outdated Show resolved Hide resolved
src/lava/proc/io/injector.py Outdated Show resolved Hide resolved
src/lava/proc/io/injector.py Outdated Show resolved Hide resolved
src/lava/proc/io/utils.py Outdated Show resolved Hide resolved
tests/lava/proc/io/test_extractor.py Outdated Show resolved Hide resolved
tests/lava/proc/io/test_injector.py Outdated Show resolved Hide resolved
tests/lava/proc/io/test_injector.py Show resolved Hide resolved
tests/lava/proc/io/test_injector.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tim-shea tim-shea left a comment

Choose a reason for hiding this comment

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

Great work Ghassen.

@gkarray gkarray merged commit 6633c93 into main Jul 10, 2023
6 checks passed
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
* First prototype of IO bridge Processes

* Progress on IO bridge Processes

* Progress on IO bridge Processes

* Progress on IO bridge Processes

* new version of processes

* have async_bridge in loihiprotocol

* tried to use PyPyChannel, serialization problem?

* PyPyChannel fix

* started renaming and cleaning up

* started renaming and cleaning up

* added tests and some input validation

* removed sync processes/models and adjusted inheritance

* add ring_queue, add fixed_point model

* few more PM tests

* started adding ring_queue in channels

* refactor in progress

* rmv ringqueue

* tests mostly finished, refactor in progress

* add extractor

* refactor in progress

* Injector Process + tests

* continue tests

* adding docstrings

* adding Extractor tests

* fix linting

* fix codacy

* refactor

* addressing change requests

* fix lint

* addressing change requests

* fix typo

* minor refactor

* minor update

---------

Co-authored-by: SveaMeyer13 <svea.meyer@tum.de>
Co-authored-by: PhilippPlank <32519998+PhilippPlank@users.noreply.github.com>
Co-authored-by: Philipp Plank <philipp.plank@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Real-time IO (Python <-> Lava)
5 participants