auto-pi-lot / autopilot

Distributed behavioral experiments
https://docs.auto-pi-lot.com
Mozilla Public License 2.0
89 stars 23 forks source link

Implement Registries - New protocols not visible #39

Closed roaldarbol closed 3 years ago

roaldarbol commented 3 years ago

I've made a dummy protocol, but was unable to see it from the Terminal's 'New Protocol' window. After some rummaging around I managed to achieve it by manually adding my new protocol to __init__.py in the Tasks directory. It would of course be great if it just appears on its own - and perhaps I've missed something in doing this. Otherwise, hereby a request to make that possible. :-D

sneakers-the-rat commented 3 years ago

YES! you are correct, this is the way it is done currently, and I don't like it and want to change it.

The idea is to use something akin to Tensor2Tensor's "registries" where you wrap the class in a decorator to register it, and can then have tasks written in a user directory rather than in the main codebase.

currently in the improvements section of the development roadmap: https://docs.auto-pi-lot.com/en/latest/todo.html#improvements

Keeping this issue open as an enhancement

arnefmeyer commented 3 years ago

I implemented a simple task registry similar to the one used in phy: https://github.com/arnefmeyer/autopilot/tree/registry

It's using metaclasses to automatically register all classes derived from "Task" (and also "Hardware" but this is not fully implemented yet). It will automatically create a "user_paths.json" file in autopilot's BASEDIR the first time autopilot is being imported. Just add user paths/classes to this file.

Maybe give it a try and let me if it works for you before I create a pull request.

sneakers-the-rat commented 3 years ago

YES! Taking a look now. This is essentially exactly what I had in mind.

I'm working on refactoring the creation of prefs to give sensible defaults, including for a user directory, so i'll merge that with your work here and a couple api tweaks from the t2t implementation and get back to you with a merged version we can check out together :)

sneakers-the-rat commented 3 years ago

Taking some (live/updating) notes on some features of registries

mikewehr commented 3 years ago

Arne this is fantastic, looking forward to giving it a try. Thanks for contributing.

arnefmeyer commented 3 years ago

@sneakers-the-rat I just added a basic hardware registry implementation. It's using decorators instead of metaclasses to avoid that hardware base classes (e.g., GPIO) end up in the pilot setup routine.

sneakers-the-rat commented 3 years ago

excellent. i'll be able to take a look later today or tomorrow, finishing up improvements to prefs and then that's next

sneakers-the-rat commented 3 years ago

made a temporary merged version between my dev and your registry branch, going to take a closer look, write tests, make a few changes based on some undocumented and v thorny history in the hardware module that i'll fill ya in on when i get a fully merged copy :) https://github.com/wehr-lab/autopilot/tree/registries

sneakers-the-rat commented 3 years ago

talk to me more about the difference in implementation between the hardware and task registries? the constraint with the setup routine and hardware is that at the time of setup (ie. before any scripts that would have been run to install hardware stuff) you can't rely on an import of that hardware module to be successful, so that's why it does the ast parsing to pull the signature. still working my way through the code, just wondering what you encountered that necessitated the different tactic

sneakers-the-rat commented 3 years ago

You know what I think i'm wrong about that, ~ thinking in the olde ways ~. I definitely need to start thinking about this as a start to a generalized plugin architecture, so the idea that some hardware submodules import some packages that aren't installed with the main package shouldn't be a problem because if something needs some additional library, system configuration, etc. then that should be specified in a plugin architecture rather than just parsed around.

sneakers-the-rat commented 3 years ago

not finished yet, taking what ya gone and done and making a generalized registry spawner, i figure we might want to make other types of plugins in the future. i haven't used python metaclasses before so this was a fun little adventure. this tiny piece of code took me much longer than i'd like to admit, but it makes it so multiple registries can be created with a similar and enforced structure

https://github.com/wehr-lab/autopilot/blob/d724202ad514b875134b197e3ac88611c15ae20b/autopilot/utils/registry.py#L28-L75

I also think the registries should be capable of importing plugins on their own, this might need to be split up but something like this i think worked, sorta a synthesis of the functions you had written. For now everything is imported as autopilot.plugins.module_name.etc but we might want to make subclases of plugins more explicit in the future. I'm shooting for just registries for now and a more concrete plugin system that integrates with environment configuration scripts in 0.5.0

https://github.com/wehr-lab/autopilot/blob/d724202ad514b875134b197e3ac88611c15ae20b/autopilot/utils/registry.py#L117-L154

also thanks for reminding me about using abc... for some reason i have this bad habit of just pretending like abstract classes don't exist and that is foolish and there's a lot of things that need to be restructured accordingly...

arnefmeyer commented 3 years ago

Is the idea of the plugin system to drop a bunch of python files into a plugin folder which are then imported as autopilot.plugins.*? I guess it might be useful to allow users to additionally specify user directories containing packages and also class paths (e.g., mypackage.mysubmodule). Python code is typically (and ideally) organized in packages rather than single python files. Moreover, might be good to register classes using their full paths (see _fullname()) rather than just their names. Think of all the possible GoNoGo tasks that live in different packages ...

Let me know when you are done with the implementation of the registry classes. I can then give it a go.

sneakers-the-rat commented 3 years ago

Definitely agree with that. I suppose I am thinking of two different things, plugins that eventually should be required to be structured like packages in a more formalized system and then a less formal "plugins folder" for one-off/esoteric classes, and this is the latter. As written if something is organized in a package structure it would be imported as such (inspect.getmodulename should pick it up) so you could get something like autopilot.plugins.myhardware.hardware1 so I should document that.

Agreed name collision is something to be concerned about, and you're right that while the module might get imported with afull module name it's currently stored as just the object name -- i'll store them as full paths and add in a little sugar to the get method that lets you get an object by just the class name if there is only one plugin class with that name. I'm thinking that we can avoid information-destroying ambiguity there by making sure that the source of every object is also saved in the resulting data file so that it's clear what plugin object and how it was implemented are clear when reading the source of the task (which should be recoverable by git hash for in-package tasks and similarly with source preservation for plugin tasks). the registries give the opportunity to finally formalize this implicit notion of 'name' or 'id' for particular instances, i've written the ability for these registries to also track class instances, which will need some anti-collision check already so maybe the same method can be used for plugin names

Ideally for the more formal plugin system we would keep some sort of registry (either in the repo or on a website or something) that points to other repositories that are structured in a particular 'plugin' formalism that includes its depends, etc. so that people would be able to select plugins from some setup menu and install them painlessly. i'm also thinking about the scripts module and how different uses require really different sets of packages, like delivering visual stimuli with psychopy requires not only a ton of additional python packages but also for the system to be reconfigured (enabling x server, etc.).

sneakers-the-rat commented 3 years ago

I also think this is related to something that' needed to happen to the networking objects for awhile, started a stub to keep track of my thoughts, but networking objects also need a registry that operates in a similar though distinct manner, keeping this in mind in this context because the registry class is extensible but the hardware and task registries currently don't really need to make much use of that, but a networking registry would need to keep track of a bunch of different things and so want to keep division of labor between registries and the networking objects themselves in mind while continuing to develop this class

https://github.com/wehr-lab/autopilot/issues/48

sneakers-the-rat commented 3 years ago

you know what this is now the defining enhancement of v0.5.0 - everything should have a registry, because everything should inherit from a root object that allows every module to have plugins. A Universal Plugin System From A Unitary Root Object. what if one wanted to implement their own networking plugin module that replaced a network node element. it should be possible to do that yes? plot objects should spawn from a registry because there should be some metaclass that can mutually coordinate their framerate if they're videos, and so the thought goes ever on.

https://github.com/wehr-lab/autopilot/issues/31

arnefmeyer commented 3 years ago

Yes, a well-defined class structure would be good. However, I don't think that everything needs a registry. At this stage it's mostly about tasks/hardware and a small number of other things (Children?) that need to be registered. Loggers, network nodes etc could be defined by every task/hardware implementation. Inheritance makes this particularly easy. Simple (incomplete) example:

import abc

"""
base classes
"""

class AutopilotObject(metaclass=abc.ABCMeta):
    pass

class Logger(AutopilotObject):
    # basic implementation
    pass

class NetworkNode(AutopilotObject):
    # basic implementation
    pass

class TaskRegistry(type(AutopilotObject)):
    pass

class Task(metaclass=TaskRegistry):

    def __init__(self, *args, **kwargs):

        self._logger = Logger()
        self._network_node = NetworkNode()

    def get_logger(self):
        return self._logger

    def get_network_node(self):
        return self._network_node

"""
user implementation
"""

class MyLogger(Logger):
    pass

class MyNetworkNode(NetworkNode):
    pass

class MyTask(Task):

    def __init__(self, *args, **kwargs):

        self._logger = MyLogger()
        self._network_node = MyNetworkNode()

if __name__ == '__main__':

    task = MyTask()
    logger = task.get_logger()
    network_node = task.get_network_node()

    print(task)
    print(logger)
    print(network_node)

The probably hardest part is to come up with a clean, minimal class structure (including class methods/properties/variables). Happy to help with that.

sneakers-the-rat commented 3 years ago

finally returned to this. life is indeed only very vaguely predictable

I played around with the metaclass implementation for awhile but ran across a few problems that I think warrant another approach... The first was that it was complicated -- sort of hard for me to reason about metaclasses, and they kept inducing more and more esoteric errors as I hunted them down. I think that would make a maintainability problem down the line. Second is that it required all the classes to be imported, which would in turn require that all hardware/etc. classes have special import logic that wraps them to ensure that they don't raise an ImportError if their dependencies aren't present. A lot of hardware objects already have this, but i also imagine this being pretty cumbersome especially if we want to support multiple plugins that depend on each other/optional dependencies for specific hardware components. To be clear, really appreciate your work drafting that one, and it definitely works, my concerns essentially just come from maintainability and compatibility with future goals I haven't clearly articulated at all!

Since we implemented the metaclass registries in the root Hardware and Task objects, it was implicit that what we were searching for was subclasses of those metaclasses, so I think a simpler alternative would be to just query subclasses. I have combined that with some of the ast parsing in the setup module to be able to look for subclasses in submodules without needing to import everything -- classes are only imported when there's a match.

will write more and finish tests and docs this week, but the draft is here: https://github.com/wehr-lab/autopilot/commit/7f54f8bdf7f2b23dd8853bc75857b6c81d61549c

and will be working on this in the simpler_registries branch.

happy 2 hear any objections :)

sneakers-the-rat commented 3 years ago

yesssssssssssssssssss https://github.com/wehr-lab/autopilot/pull/109

sneakers-the-rat commented 3 years ago

closed with https://github.com/wehr-lab/autopilot/pull/109 !