-
Notifications
You must be signed in to change notification settings - Fork 147
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
Enable int or None as run_condition for Process.run and Runtime.start #769
Enable int or None as run_condition for Process.run and Runtime.start #769
Conversation
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.
Generally very nice - I am just a little concerned about the parameter naming. I think that needs to be revisited now that it is possible to go without a specific RunConfig
object.
@@ -73,6 +88,33 @@ def test_executable_node_config_assertion(self): | |||
"Expected node_configs[0] node_config length to be 2") | |||
runtime4.stop() | |||
|
|||
def test_run_conditions(self): |
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.
You are testing four features in this one test. I would split these into separate tests, where you reuse the setup with the setUp()
method:
def setUp() -> None:
...
self.runtime = source.runtime
...
def test_implicit_run_steps_condition() -> None:
self.runtime.start(run_condition=3)
data = sink.data.get().astype(int).flatten().tolist()
self.assertEqual(data, [0, 1, 2, 3, 4, 5, 6, 7, 8, 0])
def test_implicit_run_continuous_condition() -> None:
self.runtime.start()
time.sleep(0.02)
self.runtime.pause()
data = sink.data.get().astype(int).flatten().tolist()
self.assertGreater(max(data), timestep + 10)
It may also be nice to add a little method for readability - just a thought:
def get_data(process: AbstractProcess) -> ty.List[int]:
return process.data.get().astype(int).flatten().tolist()
def test_implicit_run_steps_condition() -> None:
self.runtime.start(run_condition=3)
data = self.get_data(sink)
self.assertEqual(data, [0, 1, 2, 3, 4, 5, 6, 7, 8, 0])
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.
And unless you expect users to use the runtime.start()
method, I would test on the Process-level API
source = SourceBuffer(data=np.array(range(1000)).reshape((1, -1)))
source.run(run_cfg=Loihi2SimCfg(), condition=5)
@@ -304,7 +304,7 @@ def is_sub_proc_of(self, proc: AbstractProcess): | |||
return False | |||
|
|||
def run(self, | |||
condition: AbstractRunCondition, | |||
condition: ty.Optional[ty.Union[AbstractRunCondition, int]], |
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.
Can we fix this inconsistency while we are at it? Why is one parameter called condition
and the other run_cfg
? Any of these combinations would make more sense to me:
config
andcondition
(prefer this)run_config
andrun_condition
(okay)run_cfg
andrun_cndtn
(ugh)
I also always get confused about which is which and look it up. Maybe there is a better naming entirely.
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.
I agree completely and have considered making this change several times, unfortunately this will be breaking for user code, so we'd need to do a deprecation thing and I put it off before.
I'll look into whether it's possible to deprecate just an argument (run_cfg) and add config.
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.
Do we really need to deprecate something at this stage? I feel we could start with that when reaching Lava version 1.0...
@@ -333,8 +333,10 @@ def run(self, | |||
|
|||
Parameters | |||
---------- | |||
condition : AbstractRunCondition | |||
Specifies for how long to run the Process. | |||
condition : Optional[Union[AbstractRunCondition, int]], default=None |
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.
Just had this thought: When we have a Process with a PyProcModel
that implements the AsyncProtocol
, are we treating an iteration of the run()
method as a "timestep"? I'm generally confused about these global time steps of the RunSteps
condition when dealing with a network of Processes that run asynchronously.
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.
Unknown. I don't believe AsyncProcess is actually working for most cases. Also, AbstractRunCondition is a bit silly in that it strongly implies that it will be an object that checks a condition, and that would be a nice, flexible pattern to use, but the reality is that when you pass a RunSteps, the actual integer gets accessed in quite a few different places and it doesn't have anything like a keep_running
boolean method.
Start the runtime and run as long as the run_condition dictates. | ||
Parameters | ||
---------- | ||
run_condition : Optional[Union[AbstractRunCondition, int]], |
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.
Here it is called run_condition
rather than condition
.
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.
Yep.
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.
Again, this overload is not necessary if we do not do it at the process level.
@@ -333,8 +333,10 @@ def run(self, | |||
|
|||
Parameters | |||
---------- | |||
condition : AbstractRunCondition | |||
Specifies for how long to run the Process. | |||
condition : Optional[Union[AbstractRunCondition, int]], default=None |
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.
I am a little concerned that it may not be obvious what condition=10
means when they see this in a tutorial without context. Without knowing what a RunCondition
is, they may think 10 is some magic number.
This is my main concern with this whole feature. Maybe it can be clarified by renaming the parameter?
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.
I would also not prefer to overload. Add another API like run_for_timestep(...) and pipe that through this generic run API. But let's not overload the existing API.
I am not sure if I like this change. Calling just Maybe we can have parameters like: Also blocking and non-blocking mode need to be specified for steps, which is currently part of the condition. |
I agree that
Adding blocking also as an argument seems like it would resolve these.
Someone long ago made the decisions that RunSteps should default to blocking while RunContinuous should default to non-blocking, and while I find that not exactly obvious it has a bit of logic to it and I'm hesitant to change reasonable behavior without a good reason. |
This isn't necessary. It was just a nice to have. |
Issue Number: #768
Objective of pull request: Add logic to create a RunSteps or RunContinuous in Runtime.start when user passes an int or None respectively. This allows users to just call process.run(5) or process.run() and get useful behavior, rather than always importing RunSteps/RunContinuous.
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?
Does this introduce a breaking change?