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

Initial release of lava-dnf v0.1.0 #8

Merged
merged 121 commits into from
Nov 29, 2021
Merged

Initial release of lava-dnf v0.1.0 #8

merged 121 commits into from
Nov 29, 2021

Conversation

mathisrichter
Copy link
Contributor

@mathisrichter mathisrichter commented Nov 22, 2021

Starting the code review process of the initial release of lava-dnf v0.1.0.
Since there is a lot of code, I am doing this now already.
The following things still need to be changed - which we will hopefully be able to do tomorrow:

  • Plotting needs to be integrated. This will be based on the Monitor feature that is awaiting code review in lava-nc/lava.
  • The tutorial has to be finished with inputs and plots.
  • Multiple unit tests are currently skipped because they have non-deterministic behavior. We are looking into that.
  • init.py files have to be changed to the new convention.
  • Unit tests are currently failing because requirements.txt is depending on the last Lava release 0.1.1 rather than the current version in the repository.
  • All comments start with non-capitalized letters - I will change that, no need to remark on every instance.

The modules that need the most critical review are:

  • lava/lib/dnf/connect (a function that connects two (neuron) processes)
  • lava/lib/dnf/inputs (runtime-adaptable spike generator process)
  • lava/lib/dnf/kernels (two kernels that can be used by a convolution-like operation)
  • lava/lib/dnf/operations (operations that are used to parameterize the connect() function)

I suggest you pick one of these modules and also go through its unit tests rather than trying to cover all the code.
If you want to get an overview of how the user is supposed to use the whole thing, have a look at the tutorial (lava-dnf/tutorials/lava/lib/dnf/tutorial_dnf_101.ipynb) and the acceptance tests (lava-dnf/tests/lava/lib/dnf/acceptance).

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>
…st_ports

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>
…ented)

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>
…ean returns

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>
…change shape.

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>
Copy link

@bamsumit bamsumit left a comment

Choose a reason for hiding this comment

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

Didn't go through all files. Looks really good @lava-dnf team.

@mathisrichter
Copy link
Contributor Author

Merge and unit tests depend on PR#96 and changes to Dense in lava-nc/lava.

Signed-off-by: Mathis Richter <mathis.richter@intel.com>
@awintel
Copy link
Contributor

awintel commented Nov 28, 2021

I have not really had a chance to review the code much. It all looks really nice and polished.
However, I noticed a lot of " -> None" for functions that return nothing.
That is not wrong but also seems pretty unnecessary. Just add type annotations when there is a type.

@mathisrichter
Copy link
Contributor Author

mathisrichter commented Nov 28, 2021

I have not really had a chance to review the code much. It all looks really nice and polished. However, I noticed a lot of " -> None" for functions that return nothing. That is not wrong but also seems pretty unnecessary. Just add type annotations when there is a type.

I am adhering to the flake8 type annotations checks. I also feel that the None return types are a bit overkill. When we have some time to add type annotations everywhere in Lava, we should agree on which checks we want globally and which to ignore. At the moment I am using the ones @bamsumit suggested, assuming he is using the same for lava-dl.

pip install flake8-annotations
flake8 --ignore=ANN101,W503,E501 src/lava

Although ANN101 (type annotations for function arguments) and E501 (line length) seem important to have.
https://www.flake8rules.com/

Signed-off-by: Mathis Richter <mathis.richter@intel.com>
Signed-off-by: Mathis Richter <mathis.richter@intel.com>
@mathisrichter mathisrichter merged commit 5deb525 into lava-nc:main Nov 29, 2021
@mathisrichter mathisrichter deleted the v0.1.0 branch November 29, 2021 13:39
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.

Initial release of lava-dnf
6 participants