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

RefPort's sometimes handled a time step late #205

Merged
merged 42 commits into from Mar 2, 2022

Conversation

PhilippPlank
Copy link
Contributor

@PhilippPlank PhilippPlank commented Mar 1, 2022

Issue Number: #204

Objective of pull request: Deterministic behavior of when data sent via a RefPort is received by the target process. Occasionally it was possible that the receiving process moved to the next phase before all of its VarPorts got serviced, because sending data via a RefPort does not block. Introducing of a wait() method for RefPorts makes sure that sent data is serviced before the process finishes (which blocks advancing a time step and ensures all VarPorts on the receiving process will be serviced beforehand).

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?

  • Occasionally it was possible that the receiving process moved to the next phase before all of its VarPorts got serviced, because sending data via a RefPort does not block.

What is the new behavior?

  • Introducing of a wait() method for RefPorts makes sure that sent data is serviced before the process finishes (which blocks advancing a time step and ensures all VarPorts on the receiving process will be serviced beforehand).

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

PhilippPlank and others added 30 commits November 12, 2021 14:53
@PhilippPlank PhilippPlank self-assigned this Mar 1, 2022
@PhilippPlank PhilippPlank linked an issue Mar 1, 2022 that may be closed by this pull request
7 tasks
@PhilippPlank PhilippPlank added 1-bug Something isn't working area: magma/core Issues with something in lava/magma/core labels Mar 1, 2022
@PhilippPlank PhilippPlank added this to In progress in lava via automation Mar 1, 2022
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.

Looks fine. But you should update the docstring.

src/lava/proc/io/reset.py Outdated Show resolved Hide resolved
src/lava/magma/core/model/py/ports.py Outdated Show resolved Hide resolved
src/lava/proc/io/reset.py Outdated Show resolved Hide resolved
@bamsumit
Copy link
Contributor

bamsumit commented Mar 2, 2022

Tested it out on PN SNN notebook. The the results are consistent.

- added wait() to the dataloader
- modified docstring of wait
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.

I would still recommend to improve the docstring.
The fundamental behavior of the RefPort should be explained in the class docstring.

Approving anyways since this is just a style issue that can be fixed later. Just don't forget about it.

src/lava/magma/core/model/py/ports.py Outdated Show resolved Hide resolved
@PhilippPlank PhilippPlank merged commit b746bb7 into lava-nc:main Mar 2, 2022
lava automation moved this from In progress to Done Mar 2, 2022
@PhilippPlank PhilippPlank deleted the refport_wait branch March 2, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-bug Something isn't working area: magma/core Issues with something in lava/magma/core
Projects
lava
Done
Development

Successfully merging this pull request may close these issues.

RefPort's sometimes handled a time step late
3 participants