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

Fixing priority of channel commands in model #190

Merged
merged 39 commits into from
Feb 25, 2022

Conversation

PhilippPlank
Copy link
Contributor

@PhilippPlank PhilippPlank commented Feb 24, 2022

Issue Number: #186

Objective of pull request: Fixing errors in unit tests with Ref-/VarPort interaction

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?

  • Intermittent failure of unit test runs (see Issue).

What is the new behavior?

  • There is not an Intermittent failure of unit test runs.

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

PhilippPlank and others added 30 commits November 12, 2021 14:53
@mgkwill
Copy link
Contributor

mgkwill commented Feb 24, 2022

Thanks for submitting this @PhilippPlank!

@@ -419,7 +419,8 @@ def add_ports_for_polling(self):
if isinstance(csp_port, CspRecvPort):
def func(fvar_port=var_port):
return lambda: fvar_port
self._channel_actions.append((csp_port, func(var_port)))
self._channel_actions.insert(0,
Copy link
Contributor

Choose a reason for hiding this comment

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

So rather than appending to the _channel_actions list

self._channel_actions: ty.List[ty.Tuple[ty.Union[CspSendPort, CspRecvPort], ty.Callable]]

we are overwriting idx 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are prepending an element rather than appending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, insert is not overwriting existing elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake. 👍🏾

Appending.

@mgkwill mgkwill linked an issue Feb 24, 2022 that may be closed by this pull request
2 tasks
@mgkwill mgkwill added the 1-bug Something isn't working label Feb 24, 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.

Admittedly, I don't understand the context of this issue. But the change seems minimal. So if it works - great.

@@ -419,7 +419,8 @@ def add_ports_for_polling(self):
if isinstance(csp_port, CspRecvPort):
def func(fvar_port=var_port):
return lambda: fvar_port
self._channel_actions.append((csp_port, func(var_port)))
self._channel_actions.insert(0,
Copy link
Contributor

Choose a reason for hiding this comment

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

We are prepending an element rather than appending.

@PhilippPlank
Copy link
Contributor Author

PhilippPlank commented Feb 25, 2022

Admittedly, I don't understand the context of this issue. But the change seems minimal. So if it works - great.

To give context. If we have VarPorts in a ProcessModel and we are in management phase, the channel action of each VarPort gets added to the list of possible channel actions to listen if anyone sent something to a VarPort.

In this unit test which failed sometimes, the list of channel actions is [default "cmd" channel, first_var_port, second_var_port]

This list is then given to the select function, which picks an action as soon as something arrives on any channel.

The problem now was, that when entering the management phase we had data on both VarPort, select picks the first one to service. After that the loop starts from the beginning and it services the first VarPort.
As this takes time, the runtime_service already sent a STOP command, since it is also the last configured time step.

Now in some cases the STOP command was sent, before we reach the select again after being done with the first VarPort. Now the select has 2 channel actions to pick from, and picked the default "cmd" channel, since it comes first in the list. This stopped the execution before we had a chance to service the second VarPort and hence the unit test failed.

Solution: order the channel command list so that VarPorts come first, so we can only listen to other commands when they all have been serviced (if needed).

@mgkwill
Copy link
Contributor

mgkwill commented Feb 25, 2022

Ah yes. Prepending rather than replacing.

OK. Makes sense to me.

@mgkwill mgkwill merged commit b08a9f3 into lava-nc:main Feb 25, 2022
@PhilippPlank PhilippPlank deleted the fix_model_cmd_priority branch February 25, 2022 18:07
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
* - fixed priority of channel commands -> var_ports should be handled first, before other "cmd" commands
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix intermittent failuire of test_explicit_Ref_Var_port_write
3 participants