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

Lava top-level API #745

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

Lava top-level API #745

wants to merge 23 commits into from

Conversation

mathisrichter
Copy link
Contributor

@mathisrichter mathisrichter commented Jul 23, 2023

Issue Number:

Objective of pull request: Imports all APIs that are relevant to users at the top Lava package level, enabling most users to write their programs with a single import statement, for example

import lava as lv

lif1 = lv.LIF(shape=(100,))
dense = lv.Dense(weights=np.eye(100))
lif2 = lv.LIF(shape=(100,))
lif1.s_out.connect(dense.s_in)
dense.a_out.connect(lif2.a_in)

lif1.run(run_cfg=lv.Loihi2HwCfg(), condition=lv.RunSteps(100))
lif1.stop()

or

from lava import LIF, Dense, Loihi2HwCfg, RunSteps

lif1 = LIF(shape=(100,))
dense = Dense(weights=np.eye(100))
lif2 = LIF(shape=(100,))
lif1.s_out.connect(dense.s_in)
dense.a_out.connect(lif2.a_in)

lif1.run(run_cfg=Loihi2HwCfg(), condition=RunSteps(100))
lif1.stop()

The behavior is implemented in lava/src/lava/__init__.py. Since adding an __init__ file makes the top-level lava module a regular package rather than a namespace package, it requires adding a line from the older pkgutils-style namespace packages. This method is compatible with the implicit namespace packages we use elsewhere. If desired, the same can be done in other Lava libraries, creating the same shared folder structure Lava currently has, still distributed over multiple repositories. In those other libraries, we could then also add more imports.

I am creating this as a draft PR for now to start a discussion.

See also this discussion about a flat module hierarchy.

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?

  • Currently, users have to import every class individually, specifying the full module path. For the small example above, users would have to import
from lava.proc.lif.process import LIF
from lava.proc.dense.process import Dense
from lava.magma.core.run_configs import Loihi2SimCfg
from lava.magma.core.run_conditions import RunSteps

What is the new behavior?

  • With this PR, users could write programs using just a single import statement.
import lava as lv

See longer example at the top of the PR description.

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
@mathisrichter mathisrichter self-assigned this Jul 23, 2023
@mathisrichter
Copy link
Contributor Author

Apparently, this breaks unit tests for tutorials at the moment.

@weidel-p
Copy link
Contributor

I like this idea, it would make the lava user scripts much cleaner.

I would compartmentalize the structure a bit though. Could we have categories such as "neurons", "connections", "runtime", "learning", etc?

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.

This is an excellent idea. I disagree with @weidel-p about compartmentalizing, I think the biggest part of the benefit of this is to flatten out the 20 or 30 most useful bits in Lava with no additional namespaces needed, but I don't feel like that's a hill I'd die on.

The tutorial tests have been super flaky for me lately, but even if they were working 100% of the time, I'd be in favor of finding a different way to test them, since they're just generally way too long and complicated to make sense in a unit test suite.

@mathisrichter
Copy link
Contributor Author

I only briefly looked into the tutorial tests but can imagine that the pkgutils-style namespace I am using could be interfering with the way the tutorial-unittests are written. I'll have a look at that later and find a fix. But I would generally also be in favor of finding a different way of testing them. Maybe in a nightly test, together with tests at scale or for demos.

Copy link
Contributor

@PhilippPlank PhilippPlank left a comment

Choose a reason for hiding this comment

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

Please test compatibility with lava-loihi.

PhilippPlank and others added 11 commits August 7, 2023 14:37
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
@mathisrichter
Copy link
Contributor Author

I fixed all issues within this repository but the underlying approach seems to not support pulling additional imports in the lava modules of other repositories (e.g., lava-loihi). This point was brought up by @bamsumit in our discussion and I verified that the different module init files overwrite each other. I have not yet looked into workarounds.

@epaxon
Copy link
Contributor

epaxon commented Aug 30, 2023

Yes, I definitely like this. This is like a statement that imports everything that users might want. What I was trying to say in the meeting, though, was that not everything is compatible with Loihi 2, and so this is like an environment that is for "CPU" backend. What I was aiming for was a way to have an import statement that is particular for "Loih2" backend, and so would only import the procs and configs particularly for Loihi 2 (and Loihi 2 emulation). Or similarly there could be a statement that is only for Loihi 1 or a statement that is for some other backend. But since this is formatted as a top-level import statement, its not clear how you would extend the idea to be only for a particular backend.

@tim-shea
Copy link
Contributor

tim-shea commented Sep 5, 2023

I fixed all issues within this repository but the underlying approach seems to not support pulling additional imports in the lava modules of other repositories (e.g., lava-loihi). This point was brought up by @bamsumit in our discussion and I verified that the different module init files overwrite each other. I have not yet looked into workarounds.

Why would lava-loihi or other package modify the set of things that get imported when you do import lava? That feels very non-deterministic and weird.

If I've installed lava-dl or lava-loihi, and there are some "top-level" components that should be easy to import from those packages, let's put those things in a lava_dl.init.py or lava.dl.init.py or something like that.

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.

None yet

5 participants