search

HPInc / HP-Digital-Microfluidics

HP Digital Microfluidics Software Platform and Libraries
MIT License
3 stars 1 forks source link

Get rid of RunMode #14

Closed EvanKirshenbaum closed 9 months ago

EvanKirshenbaum commented 9 months ago

All operation scheduling currently takes two optional arguments:

mode: RunMode = RunMode.GATED
after: Optional[DelayType] = None

where 

DelayType = Union[Ticks, Time]

class RunMode:
    is_gated: Final[bool]
    motion_time: Time
    GATED: ClassVar[RunMode]

    @classmethod   
    def asynchronous(cls, motion_time: Time) -> RunMode:
        return RunMode(False, motion_time)

RunMode.GATED = RunMode(True, Time.ZERO())

The notion was that you could either run using the system clock (which could be paused) or using actual wall-clock time, and that operations such as drop movement or setting heater temperature needed to be able to run either way. The reason we needed the RunMode object was that for things like walking n steps, I anticipated scheduling n motion operations each k steps in the future, and I needed some way to tell whether this should be k clock ticks or k times some Time. So the RunMode both encapsulated this distinction (for when no after value was specified) and provided the clock interval to use for when the mode is asynchronous.

There are a couple of problems with this:

So, on balance, it seems like it would be worthwhile to simply get rid of RunMode. If a user really wants to be able to say "Walk drops at 50ms/step even though the clock interval is 100ms," we can simply make that a separate operation, implemented differently.

Migrated from internal repository. Originally created by @EvanKirshenbaum on Jun 08, 2021 at 4:50 PM PDT. Closed on Mar 21, 2022 at 2:15 PM PDT.
EvanKirshenbaum commented 9 months ago

This issue was referenced by the following commit before migration:

EvanKirshenbaum commented 9 months ago

Should calls like this:

board.before_tick(lambda: next(iterator), delta=mode.gated_delay(after))

be changed to this:

board.before_tick(lambda: next(iterator), delta=after)
Migrated from internal repository. Originally created by Rares Vernica on Mar 10, 2022 at 10:35 AM PST.
EvanKirshenbaum commented 9 months ago

How about blocks like this:

        if mode.is_gated:
            self.on_tick(cb, delta=mode.gated_delay(after))
        else:
            self.communicate(cb, delta=mode.asynchronous_delay(after))

should it be changed to just:

            self.communicate(cb, delta=after)
Migrated from internal repository. Originally created by Rares Vernica on Mar 10, 2022 at 10:38 AM PST.
EvanKirshenbaum commented 9 months ago

You say "blocks like", but I think SystemComponent.schedule() is the only place that has that idiom. I'm pretty sure I added it in when I got tired of having to put code like it all over the place. So it's probably what you should be feeding through.

My inclination would be to simply change it to something like

        if after is None or isinstance(after, Ticks):
            self.on_tick(cb, delta=after)
        else:
            self.communicate(cb, delta=after)

This has the disadvantage that if after isn't specified, what you get is gated behavior. Probably a better thing to do would be to let the system component itself decide whether it's gated or not by default by doing something like

    def schedule(self, cb: Callable[[], Optional[Callback]], mode: RunMode, *, 
                 after: Optional[DelayType] = None) -> None:
        if after is None:
            after=self.no_delay()
        if isnstance(after, Ticks):
            self.on_tick(cb, delta=after)
        else:
            self.communicate(cb, delta=after)

    def no_delay(self) -> DelayType:
       return Time.ZERO
...
class Board(SystemComponent):
   ...
   def no_delay(self) -> DelayType:
      return Ticks.ZERO

(or make it a class attribute, since it's probably not going to change).

Migrated from internal repository. Originally created by @EvanKirshenbaum on Mar 10, 2022 at 11:34 AM PST.
EvanKirshenbaum commented 9 months ago

Should calls like this:

board.before_tick(lambda: next(iterator), delta=mode.gated_delay(after))

be changed to this:

board.before_tick(lambda: next(iterator), delta=after)

Probably, but you then have to deal with the fact that before_tick (and after_tick) specify that delta is of type Ticks (and what gated_delay is doing is failing an assertion if what you have is a Time).

What you may want to do is change these two functions to take a DelayType and change System.before_tick() (which they eventually delegate to) to something on the order of

    def before_tick(self, fn: ClockCallback, *, delta: DelayType =Ticks.ZERO) -> None:
        if isinstance(delta, Ticks):
            self._channel().before_tick([(delta, fn)])
        else:
            self.call_after(delta, lambda: self.before_tick(fn))

In other words, wait for delta and then call before_tick() on the next tick. (Although thinking about it, for before_tick, you might want to subtract self.clock.update_interval from the delta and only do the call_after if this is greater than zero so the reading is "before the tick on or after delta". Maybe.)

Migrated from internal repository. Originally created by @EvanKirshenbaum on Mar 10, 2022 at 12:02 PM PST.
EvanKirshenbaum commented 9 months ago

What about System.on_tick, should we change that too?

Migrated from internal repository. Originally created by Rares Vernica on Mar 10, 2022 at 12:48 PM PST.
EvanKirshenbaum commented 9 months ago

I think I made the changes you suggested:

class SystemComponent:
...
         def before_tick(self, fn: ClockCallback, *, delta: DeltaType=Ticks.ZERO) -> None:
1558:        self.in_system().before_tick(fn, delta=delta)

class System:
         def before_tick(self, fn: ClockCallback, *, delta: DelayType=Ticks.ZERO) -> None:
             if isinstance(delta, Ticks):
                 self._channel().before_tick([(delta, fn)])
             else:
2040:            self.call_after(delta, lambda: self.before_tick(fn))

But I get a TypeError:

Exception in thread Dummy Pipettor Thread:
Traceback (most recent call last):
  File "/usr/lib64/python3.10/threading.py", line 1009, in _bootstrap_inner
    self.run()
  File "/home/vernica/proj/thylacine/mpam/src/mpam/types.py", line 1449, in run
    func()
  File "/home/vernica/proj/thylacine/mpam/src/mpam/pipettor.py", line 175, in run_it
    self.pipettor.perform(xfer)
  File "/home/vernica/proj/thylacine/mpam/src/devices/dummy_pipettor.py", line 114, in perform
    x.finished(reagent, x.volume)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/pipettor.py", line 57, in finished
    self.future.post(liquid)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/types.py", line 700, in post
    fn(val)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/types.py", line 663, in <lambda>
    self.when_value(lambda val: other.post(transform(val)))
  File "/home/vernica/proj/thylacine/mpam/src/mpam/types.py", line 700, in post
    fn(val)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/types.py", line 649, in <lambda>
    self.when_value(lambda val: other.post(val))
  File "/home/vernica/proj/thylacine/mpam/src/mpam/types.py", line 700, in post
    fn(val)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/types.py", line 339, in schedule_and_post
    f = self._schedule_for(x, after=after, post_result=post_result)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/drop.py", line 699, in _schedule_for
    board.before_tick(lambda: next(iterator), delta=after)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/device.py", line 1558, in before_tick
    self.in_system().before_tick(fn, delta=delta)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/device.py", line 2040, in before_tick
    self.call_after(delta, lambda: self.before_tick(fn))
  File "/home/vernica/proj/thylacine/mpam/src/mpam/device.py", line 2034, in call_after
    self._channel().call_after([(delta, fn, daemon)])
  File "/home/vernica/proj/thylacine/mpam/src/mpam/engine.py", line 151, in call_after
    self.timer_thread.call_after(reqs)
  File "/home/vernica/proj/thylacine/mpam/src/mpam/engine.py", line 432, in call_after
    self.call_at([(now+delta, fn, daemon) for (delta, fn, daemon) in reqs])
  File "/home/vernica/proj/thylacine/mpam/src/mpam/engine.py", line 432, in <listcomp>
    self.call_at([(now+delta, fn, daemon) for (delta, fn, daemon) in reqs])
  File "/home/vernica/proj/thylacine/mpam/src/quantities/timestamp.py", line 30, in __add__
    return Timestamp(self.time+rhs)
  File "/home/vernica/proj/thylacine/mpam/src/quantities/core.py", line 362, in __add__
    self._ensure_dim_match(rhs, "+")
  File "/home/vernica/proj/thylacine/mpam/src/quantities/core.py", line 357, in _ensure_dim_match
    raise TypeError(f"RHS of {op} is not a Quantity: {rhs}")
TypeError: RHS of + is not a Quantity: None
Migrated from internal repository. Originally created by Rares Vernica on Mar 10, 2022 at 1:22 PM PST.
EvanKirshenbaum commented 9 months ago

Pushed current changes in this branch When running pcr I get the error listed above.

Migrated from internal repository. Originally created by Rares Vernica on Mar 10, 2022 at 1:32 PM PST.
EvanKirshenbaum commented 9 months ago

I strongly, strongly suggest you use a development tool that will run MyPy for you (or figure out how to get it incorporated into your tool or run it manually).

Looking at the code you pushed, in the definitions of SystemComponent.before_tick and .after_tick (lines 1557 and 1560), you specified the type of delta to be DeltaType rather than DelayType, while in the definition of System.on_tick (line 2048), you have it as Delay.

These errors mean that in MotionOp._schedule_for (drop.py, line 699), it couldn't tell you that you we passing in an Optional[DelayType] to something that was (or should have been) spec'd to take a DelayType, and the actual argument was None. As a result, in TimerThread.call_after (engine.py, line 432), it's trying to add a Timestamp and a None, which doesn't work.

If we're just going to be passing the after params through to SystemComponent, we probably should make before_tick's delta be Optional and change it to be Ticks.ZERO if it's None, e.g.,, something like

    def before_tick(self, fn: ClockCallback, *, delta: Optional[DelayType]=Ticks.ZERO) -> None:
        if delta is None:
            delta = Ticks.ZERO
        self.in_system().before_tick(fn, delta=delta)
Migrated from internal repository. Originally created by @EvanKirshenbaum on Mar 10, 2022 at 2:37 PM PST.
EvanKirshenbaum commented 9 months ago

OK, I fixed the typo and re-run but still got the TypeError.

I made the extra changes you suggested and pcr ran successfully.

Did not know about MyPy. Happy to run in on the command line. In the future we can also add it to GitHub Actions with any other tests already existing. I ran MyPy on the device.py and got 28 error but probably not relevant:

> mypy src/mpam/device.py                           [0]
src/mpam/types.py:11: error: Skipping analyzing "matplotlib._color_data": module is installed, but missing library stubs or py.typed marker
src/mpam/types.py:14: error: Cannot find implementation or library stub for module named "_collections"
src/mpam/types.py:1324: error: Skipping analyzing "matplotlib": module is installed, but missing library stubs or py.typed marker
src/langsup/type_supp.py:9: error: Cannot find implementation or library stub for module named "_collections"
src/mpam/device.py:13: error: Skipping analyzing "matplotlib.gridspec": module is installed, but missing library stubs or py.typed marker
src/mpam/device.py:13: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
src/mpam/device.py:30: error: Cannot find implementation or library stub for module named "_collections"
src/mpam/device.py:2052: error: Name "Delay" is not defined
src/mpam/drop.py:3: error: Cannot find implementation or library stub for module named "_collections"
src/mpam/processes.py:13: error: Cannot find implementation or library stub for module named "_collections"
src/langsup/dmf_lang.py:3: error: Cannot find implementation or library stub for module named "_collections"
src/langsup/dmf_lang.py:10: error: Skipping analyzing "antlr4": module is installed, but missing library stubs or py.typed marker
src/langsup/dmf_lang.py:12: error: Skipping analyzing "antlr4.tree.Tree": module is installed, but missing library stubs or py.typed marker
src/langsup/dmf_lang.py:14: error: Cannot find implementation or library stub for module named "DMFLexer"
src/langsup/dmf_lang.py:15: error: Cannot find implementation or library stub for module named "DMFParser"
src/langsup/dmf_lang.py:16: error: Cannot find implementation or library stub for module named "DMFVisitor"
src/mpam/monitor.py:13: error: Skipping analyzing "clipboard": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:14: error: Skipping analyzing "matplotlib": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:15: error: Skipping analyzing "matplotlib.artist": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:16: error: Skipping analyzing "matplotlib.axes._axes": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:17: error: Skipping analyzing "matplotlib.backend_bases": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:18: error: Skipping analyzing "matplotlib.figure": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:19: error: Skipping analyzing "matplotlib.gridspec": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:20: error: Skipping analyzing "matplotlib.legend": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:21: error: Skipping analyzing "matplotlib.legend_handler": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:22: error: Skipping analyzing "matplotlib.patches": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:23: error: Skipping analyzing "matplotlib.path": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:24: error: Skipping analyzing "matplotlib.text": module is installed, but missing library stubs or py.typed marker
src/mpam/monitor.py:25: error: Skipping analyzing "matplotlib.widgets": module is installed, but missing library stubs or py.typed marker
Found 28 errors in 7 files (checked 1 source file)

Let me know if anything else is needed here.

Migrated from internal repository. Originally created by Rares Vernica on Mar 10, 2022 at 5:27 PM PST.
EvanKirshenbaum commented 9 months ago

Sigh. Yeah, this stuff needs to be captured somewhere, since it doesn't appear to be the sort of thing that you can easily make part of the repo. There are two different issues here.

First off, the "missing library stubs" errors. Looking at my Eclipse config, I use the following arguments to MyPy:

--follow-imports=silent --show-column-numbers --ignore-missing-imports
--warn-redundant-casts --warn-return-any --warn-unreachable
--strict-equality --show-error-codes

MyPy uses the type hints in the code to do its checking, but for legacy code that doesn't have hints, you can add a .pyi "stub" file (akin to a C++ .h) that gives the type hints but no implementation. clipboard, "antlr4", and matplotlib don't have them, and the --ignore-missing-imports argument tells MyPy not to bother you about them. (You just have to be more careful writing code that uses those libraries, because you won't get warned if you do something that isn't typesafe.)

The second problem is that it can't find the code for the "_collections" library or the ANTLR-emitted code (which is in target/generated-sources/antlr4.) Again, looking at my Eclipse config, I've got a check next to Add project source folders to MYPYPATH?, so you might look into that. My source folders are

That should get the "DMF..." files. I don't know why it can't find _collections. Or, more to the point, I'm not sure why the automatically-generated import decided to pull from _collections rather than collections, although I see that on my system, collections just turns around and tries to import defaultdict from _collections. at least in my 3.9 version. You might try changing the import statements to remove the underscore and see if that helps.

Migrated from internal repository. Originally created by @EvanKirshenbaum on Mar 11, 2022 at 10:41 AM PST.
EvanKirshenbaum commented 9 months ago

OK, that helps. I only needed one of the MyPy arguments from your setup. I ran it on all the files that I change in my branch. There is one outstanding error. I poked around, but it does not look straightforward to fix.

> mypy --ignore-missing-imports src/mpam/{device,drop,paths,pipettor,processes,types}.py tests/{joey,od,wombat}_test.py tools/{bilby,wombat}.py
src/mpam/device.py:2053: error: List item 0 has incompatible type "Tuple[Union[Ticks, Time], Callable[[], Iterable[Updatable]]]"; expected "Tuple[Ticks, Callable[[], Iterable[Updatable]]]"
Found 1 error in 1 file (checked 11 source files)
Migrated from internal repository. Originally created by Rares Vernica on Mar 17, 2022 at 10:56 AM PDT.
EvanKirshenbaum commented 9 months ago

What does that line look like for you? In the most recent code you pushed to the issue.014 branch, it looks like

    def call_at(self, t: Timestamp, fn: TimerFunc, *, daemon: bool = False) -> None:
        self._channel().call_at([(t, fn, daemon)])

which doesn't look to be susceptible to that error.

Migrated from internal repository. Originally created by @EvanKirshenbaum on Mar 17, 2022 at 12:00 PM PDT.
EvanKirshenbaum commented 9 months ago

I since merged the latest changes from the master. The error is still there, but at an updated line number.

> git status
On branch issue.014
Your branch is up to date with 'origin/issue.014'.
> mypy --ignore-missing-imports src/mpam/{device,drop,paths,pipettor,processes,types}.py tests/{joey,od,wombat}_test.py tools/{bilby,wombat}.py
src/mpam/device.py:2072: error: List item 0 has incompatible type "Tuple[Union[Ticks, Time], Callable[[], Iterable[Updatable]]]"; expected "Tuple[Ticks, Callable[[], Iterable[Updatable]]]"
Found 1 error in 1 file (checked 11 source files)

Here are all call_at declarations:

> grep -R "def call_at"
src/mpam/device.py:    def call_at(self, t: Timestamp, fn: TimerFunc, *, daemon: bool = False):
src/mpam/device.py:    def call_at(self, reqs: Sequence[TimerRequest]) -> None:
src/mpam/device.py:    def call_at(self, t: Timestamp, fn: TimerFunc, *, daemon: bool = False) -> None:
src/mpam/engine.py:    def call_at(self, reqs: Sequence[TimerRequest]) -> None:
src/mpam/engine.py:    def call_at(self, reqs: Sequence[TimerRequest]) -> None:

Specifically, the one from Batch is:

    # Line 1960 in device.py
    def call_at(self, reqs: Sequence[TimerRequest]) -> None:
        self.buffer_call_at.extend(reqs)

and the one from Engine is:

    # Line 147 in engine.py
    def call_at(self, reqs: Sequence[TimerRequest]) -> None:
        self.timer_thread.call_at(reqs)

I think one of these two is called from System.on_tick where the error is.

Migrated from internal repository. Originally created by Rares Vernica on Mar 17, 2022 at 12:26 PM PDT.
EvanKirshenbaum commented 9 months ago

I've merged in the pull request. This one looks to be done.

Migrated from internal repository. Originally created by @EvanKirshenbaum on Mar 21, 2022 at 2:15 PM PDT.