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

Enable int or None as run_condition for Process.run and Runtime.start #769

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tim-shea
Copy link
Contributor

@tim-shea tim-shea commented Aug 5, 2023

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:

  • 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 (flakeheaven lint src/lava tests/) and (bandit -r src/lava/.) pass locally
  • Build tests (pytest) passes locally

Pull request type

Please check your PR type:

  • Feature

What is the current behavior?

  • User must always explicitly create an instance of AbstractRunCondition.

What is the new behavior?

  • Runtime.start will implicitly create a RunSteps or RunContinuous if condition is an int or None.

Does this introduce a breaking change?

  • No

@tim-shea tim-shea self-assigned this Aug 5, 2023
@tim-shea tim-shea added this to the Lava v0.8.1 milestone Aug 5, 2023
Copy link
Contributor

@mathisrichter mathisrichter left a 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):
Copy link
Contributor

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])

Copy link
Contributor

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]],
Copy link
Contributor

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 and condition (prefer this)
  • run_config and run_condition (okay)
  • run_cfg and run_cndtn (ugh)

I also always get confused about which is which and look it up. Maybe there is a better naming entirely.

Copy link
Contributor Author

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.

Copy link
Contributor

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

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.

Copy link
Contributor Author

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]],
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.

Copy link
Contributor

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

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?

Copy link
Contributor

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.

@PhilippPlank
Copy link
Contributor

PhilippPlank commented Aug 7, 2023

I am not sure if I like this change. Calling just <some_proc>.run() does not tell what is going to happen. Also <some_proc>.run(10) or <some_proc>.run(condition=10).

Maybe we can have parameters like:
<some_proc>.run(steps=10)
or
<some_proc>.run(continuous=True) (although False would not really make sense, but I cannot think of a good way to start a continuous run without importing something)

Also blocking and non-blocking mode need to be specified for steps, which is currently part of the condition.

@tim-shea
Copy link
Contributor Author

tim-shea commented Aug 7, 2023

I am not sure if I like this change. Calling just <some_proc>.run() does not tell what is going to happen. Also <some_proc>.run(10) or <some_proc>.run(condition=10).

I agree that proc.run(10) is a bit weird and it would be better to add a param called steps. Agree to disagree that proc.run() is ambiguous. It does exactly what it says on the tin: runs.

Maybe we can have parameters like: <some_proc>.run(steps=10) or <some_proc>.run(continuous=True) (although False would not really make sense, but I cannot think of a good way to start a continuous run without importing something)

Also blocking and non-blocking mode need to be specified for steps, which is currently part of the condition.

Adding blocking also as an argument seems like it would resolve these.

  • proc.run() would be non-blocking, continuous.
  • proc.run(blocking=False) same as previous
  • proc.run(blocking=True) would be blocking, continuous. Perhaps should log a warning or info along the lines of "Running a process in blocking, continuous mode. Note that one or more connected processes must request a stop or pause or this will run forever."
  • proc.run(steps=N) would be blocking, num-steps
  • proc.run(steps=N, blocking=True) same as previous
  • proc.run(steps=N, blocking=False) would be non-blocking, num-steps

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.

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.

Process.run and Runtime.start should accept an int or None as conditions
4 participants