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.

Sign up for GitHub

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

Jump to bottom

Implementation of Pause and Run Continuous with refactoring of Runtime #171

Merged
merged 16 commits into from
Feb 17, 2022
Merged

Implementation of Pause and Run Continuous with refactoring of Runtime #171

merged 16 commits into from
Feb 17, 2022

Conversation

ysingh7
Copy link
Contributor

@ysingh7 ysingh7 commented Feb 8, 2022

Issue Number: 86, 53

Objective of pull request: Implementation of Pause and Run Continuous with refactoring of Runtime

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 (pyb) passes locally
  • Build tests (pyb -E unit) or (python -m unittest) 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?

What is the new behavior?

  • Pause, Run Continuous, AsynchronousProcessModel were implemented

Does this introduce a breaking change?

  • Yes
  • No

AbstractProcessModel has been refactored which will change most of the user defined ProcessModel.

Supplemental information

awintel
awintel previously requested changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@awintel awintel left a comment

Choose a reason for hiding this comment

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

Great that this is implemented.

However, you should work on some of the docstrings. In particular there seem to be a couple of automatic renaming errors scattered throughout the whole code base in this PR that make absolutely no sense when you read the resulting docstrings.

tests/lava/magma/runtime/test_run_continuously_and_pause.py Show resolved Hide resolved
tests/lava/magma/runtime/test_run_continuously_and_pause.py Show resolved Hide resolved
tests/lava/magma/runtime/test_run_continuously_and_pause.py Outdated Show resolved Hide resolved
tests/lava/magma/runtime/test_run_continuously_and_pause.py Show resolved Hide resolved
tests/lava/magma/runtime/test_run_continuously_and_pause.py Show resolved Hide resolved
src/lava/magma/runtime/runtime.py Show resolved Hide resolved
src/lava/magma/runtime/runtime.py Outdated Show resolved Hide resolved
src/lava/magma/runtime/runtime.py Show resolved Hide resolved
src/lava/magma/runtime/runtime.py Outdated Show resolved Hide resolved
src/lava/magma/runtime/runtime.py Show resolved Hide resolved
@ysingh7 ysingh7 requested a review from awintel February 17, 2022 01:00
@ysingh7 ysingh7 dismissed awintel’s stale review February 17, 2022 17:47

I have made most of the changes and have noted down what I haven't done. I need to merge this pull request.

mgkwill
mgkwill reviewed Feb 17, 2022
View reviewed changes
tests/lava/proc/conv/test_models.py
@@ -92,6 +92,7 @@ def setup_conv() -> Tuple[
class TestConvProcessModels(unittest.TestCase):
"""Tests for all ProcessModels of Conv"""

@unittest.skip
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for skipping test_conv_float and test_conv_fixed?

mgkwill
mgkwill reviewed Feb 17, 2022
View reviewed changes
tests/lava/proc/io/test_source_sink.py
@@ -75,66 +57,3 @@ def test_source_sink(self) -> None:
f'{output[output!=input]=}\n'
f'{input[output!=input] =}\n'
)

def test_read(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also why remove test_read and test_reset?

@mgkwill
Copy link
Contributor

mgkwill commented Feb 17, 2022

I don't know why the MacOs unit test run is taking 25m plus. I'm restarting CI run to check if this is persistent.

This was linked to issues Feb 17, 2022
Runtime deadlocks when ProcessModel building or execution fails for any reason without any exceptions thrown at user #83
Closed
Enable PAUSE command in runtime #53
Closed
@mgkwill
Copy link
Contributor

mgkwill commented Feb 17, 2022

Third time's a charm. Seems there is some intermittent failure for MacOS unit test:

======================================================================
FAIL: test_explicit_Ref_Var_port_write (tests.lava.magma.runtime.test_ref_var_ports.TestRefVarPorts)
Tests the connection of a RefPort to an explicitly created VarPort.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/lava/lava/tests/lava/magma/runtime/test_ref_var_ports.py", line [120](https://github.com/lava-nc/lava/runs/5238112313?check_suite_focus=true#step:5:120), in test_explicit_Ref_Var_port_write
    self.assertTrue(np.all(recv.var2.get() == np.array([7., 7., 7.])))
AssertionError: False is not true

@mgkwill mgkwill merged commit 4956db0 into lava-nc:main Feb 17, 2022
@mgkwill
Copy link
Contributor

mgkwill commented Feb 17, 2022

Third time's a charm. Seems there is some intermittent failure for MacOS unit test:

======================================================================
FAIL: test_explicit_Ref_Var_port_write (tests.lava.magma.runtime.test_ref_var_ports.TestRefVarPorts)
Tests the connection of a RefPort to an explicitly created VarPort.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/lava/lava/tests/lava/magma/runtime/test_ref_var_ports.py", line [120](https://github.com/lava-nc/lava/runs/5238112313?check_suite_focus=true#step:5:120), in test_explicit_Ref_Var_port_write
    self.assertTrue(np.all(recv.var2.get() == np.array([7., 7., 7.])))
AssertionError: False is not true

Filed #186 for the above failure. It doesn't seem related to code in this PR.

monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
@ysingh7
Implementation of Pause and Run Continuous with refactoring of Runtime (
805ce46 
lava-nc#171)

* Implementation of the refactored runtime with pause and run-continous feature

* Fixed the constructor of ProcessModel

* Fixed the issue with missing HostCPU

* Made set_var blocking call

* Fixed a bug in runtime service which was causing monitor tests to fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgkwill mgkwill mgkwill approved these changes

@joyeshmishra joyeshmishra joyeshmishra approved these changes

@bamsumit bamsumit Awaiting requested review from bamsumit

@PhilippPlank PhilippPlank Awaiting requested review from PhilippPlank

@srrisbud srrisbud Awaiting requested review from srrisbud

@awintel awintel Awaiting requested review from awintel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ysingh7 @mgkwill @joyeshmishra @awintel