lava icon indicating copy to clipboard operation
lava copied to clipboard

Profiler depends on Loihi2ProfilerApi and has other problems

Open tim-shea opened this issue 3 years ago • 2 comments

Objective of issue: Profiler currently attempts to import the class Loihi2ProfilerApi which is defined in lava-loihi and throws an error if it can't be imported. This creates a highly undesirable circular dependency. In addition, the docstring for Profiler suggests that it's a base class for the actual hardware profilers, but in reality it's just an abstract factory for creating Profilers which don't necessarily share any base class.

  1. Profiler should be refactored to eliminate the dependency on Loihi2ProfilerApi. It should raise a NotImplementedError when a method is called on the base class which hasn't been overridden by a hardware-specific profiler.
  2. The Profiler factory function can throw an exception when a RunConfig is passed and no matching Profiler is found, there is no need to throw this error on import.
  3. Profiler should probably be a base class for the actual profilers. Otherwise there's no reason for it to exist.
  4. Profiler should probably be a process, because it exists during runtime and it consumes and produces data, thus it's performance where and when it runs should be manageable using the same tools as everything else.

Lava version:

  • [X] 0.6.0 (feature release)

I'm submitting a ...

  • [X] bug report

Current behavior: Profiler depends on Loihi2ProfilerApi, it's not a process, it's not a base class for the Loihi2Profiler.

Expected behavior: See description.

tim-shea avatar Nov 18 '22 05:11 tim-shea

@bamsumit @PhilippPlank

tim-shea avatar Nov 18 '22 05:11 tim-shea

The rationale behind the factory instead of a base class is, that it is very likely that every profiler has different capabilities. Sure, time, power and energy is probably a common thing, but the granular information most likely going to be different for each profiler implementation. So a base class with common methods does not make sense, as the profilers just will not be that common.

I am open for a solution which chooses the profiler based on the RunCfg without having to import the profiler class.

PhilippPlank avatar Nov 18 '22 10:11 PhilippPlank