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

Allow Scan to take no parameters #62

Open
evalott100 opened this issue Oct 15, 2024 · 8 comments
Open

Allow Scan to take no parameters #62

evalott100 opened this issue Oct 15, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@evalott100
Copy link
Contributor

evalott100 commented Oct 15, 2024

Currently Scan is only validating correctly if initialised through the scan decorator:

if not len(self.parameters) == 1:
raise FastCSException("Scan method cannot have arguments")

This is because inspect.Signature.parameters will ignore self, but not in the wrapped case:

import inspect

class SomeClass:
    def __init__(self, fn, b):
        print(f"someclass fn {fn.__name__}", inspect.signature(fn).parameters)

def dec(b: str):
    def wrapper(fn):
        fn.wrapped_foo = SomeClass(fn, b)
    return wrapper

class X:
    def __init__(self):
        print("internal foo", inspect.signature(self.foo).parameters)
        print("internal bar", inspect.signature(self.bar).parameters)
        SomeClass(self.foo, "c")

    async def foo(self): ...
    async def bar(self, a: str): ...

    @dec("b")
    async def wrapped_foo(self): ...

x = X()

gives us

someclass fn wrapped_foo OrderedDict([('self', <Parameter "self">)])
internal foo OrderedDict()
internal bar OrderedDict([('a', <Parameter "a: str">)])
someclass fn foo OrderedDict()

I would like to be able to pass a poll period into my controller at init time

class PandaController(Controller):
    def __init__(self, hostname: str, poll_period: float) -> None:
        ...
        self.fastcs_method = Scan(self.update, poll_period)

but this leads to no parameters and the check above failing because the expected self argument isn't present.

Suggestion

Change

if not len(self.parameters) == 1:
raise FastCSException("Scan method cannot have arguments")

to

        if self.parameters and tuple(self.parameters.keys()) != ("self",):
            raise FastCSException("Scan method cannot have arguments")
@evalott100 evalott100 added the bug Something isn't working label Oct 15, 2024
@GDYendell
Copy link
Contributor

GDYendell commented Oct 15, 2024

It is intended that methods of Controllers have fastcs_method added so that they get processed as Methods by the backend (in the case of Scan, to create a loop that calls the given method at the given rate), not to add it to the Controller itself. What are you hoping to happen by adding self.fastcs_method to Controller?

@evalott100
Copy link
Contributor Author

evalott100 commented Oct 15, 2024

It was my understanding that on Backend.__init__, Mapping is initialised and fields in the Controller wrapped in Scan would be picked up in the walk and have their scan set up in Backend._start_scan_tasks?

My aim is really just to have that method be picked up as a Scan, but without using the decorator since --poll-period being an argument means it has to be passed in on the child Controller init.

@GDYendell
Copy link
Contributor

GDYendell commented Oct 16, 2024

Ah I see. No it does not find fields that are Scan, it finds methods that have an attribute fastcs_method that is a Scan (here).

So, if you want to do this dynamically from a CLI argument you would need to make a method (or a Callable) in __init__ with the ScanCallback signature and attach a Scan(dynamic_period) to it. See how that goes. If that doesn't work or is horrible, we might need to rethink the API.

@evalott100
Copy link
Contributor Author

evalott100 commented Oct 16, 2024

Initially I misread and did something like

self.fastcs_method = Scan(self.update, poll_period)

in __init__ of my controller, however going by the wrapper I should instead be adding the attribute fastcs_method to the update, however you can't do that on an object method, only the class... so

# Globally
PandaController.update.fastcs_method = Scan(PandaController.update, 0.1)

Works and sets up the scan correctly - the signature parameters see self len(parameters) == 1.

Edit

I can get around it with the following:

        class Updater:
            fastcs_method = Scan(PandaController.update, poll_period)
        self.updater = Updater()

@GDYendell
Copy link
Contributor

GDYendell commented Oct 16, 2024

Can you not just do self.update.fastcs_method = Scan(self.update, poll_period) in __init__?

@evalott100
Copy link
Contributor Author

evalott100 commented Oct 16, 2024

Can you not just do self.update.fastcs_method = Scan(self.update, poll_period) in __init__?

Unfortunately not, you can't add attributes to methods on objects, only methods on classes:

from pprint import pprint
class Foo:
    def bar(self):
        pass

setattr(Foo.bar, "some_string", "meh")
setattr(Foo().bar, "some_string", "meh") # Raises attribute error

@GDYendell
Copy link
Contributor

Ah, right. You can do

setattr(Foo().__class__.bar, "some_string", "meh")

but that is pretty horrible.

I have recently tried refactoring this so that the decorator replaces the method with a callable Scan, but I still couldn't convince pyright strict, so I shelved that for now. But maybe it would be an improvement even if it doesn't satisfy pyright. Then I think you could do

self.update = Scan(self.update, period)

@GDYendell
Copy link
Contributor

I have reworked my WIP here. See the test for an example of creating a BoundCommand from an instance method. BoundScan should work in the same way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants