-
Notifications
You must be signed in to change notification settings - Fork 144
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
base: main
Are you sure you want to change the base?
Lava top-level API #745
Conversation
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Apparently, this breaks unit tests for tutorials at the moment. |
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? |
There was a problem hiding this 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.
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. |
There was a problem hiding this 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.
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
Signed-off-by: Mathis Richter <[email protected]>
I fixed all issues within this repository but the underlying approach seems to not support pulling additional imports in the |
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. |
Why would lava-loihi or other package modify the set of things that get imported when you do 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. |
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
or
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:
flakeheaven lint src/lava tests/
) and (bandit -r src/lava/.
) pass locallypytest
) passes locallyPull request type
Please check your PR type:
What is the current behavior?
What is the new behavior?
See longer example at the top of the PR description.
Does this introduce a breaking change?
Supplemental information