WIP: pyscript decorators

[dlashua]

This PR allows for decorators from pyscript functions.

1. I have no idea if there's a better way to do this.
2. This has only limited testing performed
3. FIXED the decorator closures are evaluated every time the function is called. this differs from standard python. I believe this is just how pyscript works (creates a new EvalFunc object each time a method is called), though, perhaps @craigbarratt can shed some light on a better place to put this that would work as expected. I can't think of any useful situations where evaluating the decorators every time would break a user's code, it's just not as efficient.

[dlashua]

This code:

class TestState:
	def __init__(self, entity, the_state):
		self.entity = entity
		self.state = the_state
		log.info(f"init for {entity} with {the_state}")

	def __call__(self, func):
		log.info("call")
		state_expr = f"{self.entity} == '{self.state}'"

		log.info(f"@state_trigger {state_expr}")

		@state_trigger(state_expr)
		def inner(*args, **kwargs):
			return func(*args, **kwargs)

		return inner

@TestState('input_boolean.test_1','on')
def dothing():
	input_boolean.test_2.toggle()

... shows that TestState is never initialized, let alone called. Which makes sense, now that I see it. Because I am evaluating the decorators when the function is CALLED and this function dothing is never called because it doesn't have a trigger (yet), the decorators are never evaluated.

So, this shows that the code to evaluate decorators needs to go somewhere else. I'm just not sure where.

[dlashua]

This code now seems to work perfectly:

def log_dec(note):
	log.info(f"log_dec init with {note}")

	def inner_dec(*args):
		log.info(f"inner_dec called with {args}")
		func = args[0]

		log.info(f"inner_dec init {func}")

		nonlocal note

		def inner(*args, **kwargs):
			log.info(f"NOTE: {note}")

			return func(*args, **kwargs)

		return inner

	return inner_dec

def log_a(func):
	def inner(*args, **kwargs):
		log.info("AAAAAAA")
		return func(*args, **kwargs)

	return inner

@state_trigger('input_boolean.test_1 == "on"')
@log_dec('this decorates dothing')
def dothing():
	log.info('doing toggle')
	input_boolean.test_2.toggle()
	log_a_note('some note')


@log_dec('this decorates log_a_note')
@log_a
def log_a_note(note):
	log.info(f"From log_a_note: {note}")

Trying to work out object based decorators now, though, I think it will require quite a bit of code to jump through all the pieces required.

[dlashua]

For reference, my goal in this is that I already have this:

def lamps_on():
    light.living_lamp_nook.turn_on()
    light.living_lamp_window.turn_on()
    input_select.living_mode.select_option(option="occupied")
rh.ZoozSwitch('light.living_overhead').on_button_press('up:2', lamps_on)

But I want to do it this way instead:

@rh.ZoozSwitch('light.living_overhead').on_button_press('up:2')
def lamps_on():
    light.living_lamp_nook.turn_on()
    light.living_lamp_window.turn_on()
    input_select.living_mode.select_option(option="occupied")

Or even:

living_overhead = rh.ZoozSwitch('light.living_overhead)

@living_overhead.on_button_press('up:2')
def blah()
  pass

@living_overhead.on_button_press('down:2')
@living_overhead.on_button_press('up:4')
def another_blah():
  pass

Syntax like that just looks better, reads better, and feels more "pyscripty" to me.

[dlashua]

@craigbarratt this seems to be the best I can get it. Thought I'm not really keen about the hacky bit here ( 907cc50#diff-733b200d9ce0361a3582c281043fd548b057e971f927f73aed75bfdc8422f06cR578-R587 ). If you have some time to look this over, maybe you can offer insight regarding a better approach or and easier, less hacky way, to do what I'm doing.

This code seems to work fine, however, in my test environment. Though it could certainly use a lot more testing. I've yet to move this into "production" to really test it because it has the potential to blow up in a difficult to trace way since I have SO many pyscripts running.

[craigbarratt]

Great - I should have time to review it tomorrow.

[craigbarratt]

Thanks for pushing forward on this. I started with your code and tweaked it some.

I'd recommend rebasing to get the latest version of call_func, which now handles async too.

Next, we need a good way to partition the hard-coded trigger decorators, so I added this near the top of the file:

TRIG_DECORATORS = {
    "time_trigger",
    "state_trigger",
    "event_trigger",
    "mqtt_trigger",
    "state_active",
    "time_active",
    "task_unique",
}

ALL_DECORATORS = TRIG_DECORATORS.union({"service"})

and replaced the existing inline trig_decorators definition with TRIG_DECORATORS.

Next, I reverted the real_call and perform_call to just update call.

Finally, the eval_decorators() function can be much simpler:

    async def eval_decorators(self, ast_ctx):
        """Evaluate the function decorators arguments."""
        self.decorators = []
        code_str, code_list = ast_ctx.code_str, ast_ctx.code_list
        ast_ctx.code_str, ast_ctx.code_list = self.code_str, self.code_list

        dec_funcs = []
        for dec in self.func_def.decorator_list:
            if isinstance(dec, ast.Call) and isinstance(dec.func, ast.Name) and dec.func.id in ALL_DECORATORS:
                args = [await ast_ctx.aeval(arg) for arg in dec.args]
                kwargs = {keyw.arg: await ast_ctx.aeval(keyw.value) for keyw in dec.keywords}
                if len(kwargs) == 0:
                    kwargs = None
                self.decorators.append([dec.func.id, args, kwargs])
            elif isinstance(dec, ast.Name) and dec.id in ALL_DECORATORS:
                self.decorators.append([dec.id, None, None])
            else:
                dec_funcs.append(await ast_ctx.aeval(dec))

        for func in reversed(dec_funcs):
            self.call = await ast_ctx.call_func(func, None, self.call)

        ast_ctx.code_str, ast_ctx.code_list = code_str, code_list

I tried testing with this decorator:

def repeat(num_times):
    def decorator_repeat(func):
        def wrapper_repeat(*args, **kwargs):
            for _ in range(num_times):
                value = func(*args, **kwargs)
            return value
        return wrapper_repeat
    return decorator_repeat

but that exposes a bug that closures don't resolve nonlocal variables if they aren't in the immediate outer scope. I need to fix that. So the workaround is this (just like you did in your example):

def repeat(num_times):
    def decorator_repeat(func):
        nonlocal num_times
        def wrapper_repeat(*args, **kwargs):
            for _ in range(num_times):
                value = func(*args, **kwargs)
            return value
        return wrapper_repeat
    return decorator_repeat

That seems to work correctly. I haven't tried object method decorators or your more complex example.

Also, one of the tests in test_unit_eval.py has a fake decorator, which fails now that there are real decorators.

[dlashua]

opening

[dlashua]

ah ha! using ast_ctx.call_func() instead of all the looping I was doing made this much much cleaner.

However, the problem that I created the hacky method for still exists.

def test_state(entity, the_state):
	state_expr = f"{entity} == '{the_state}'"

	def test_state_inner_dec(funcxx):
		nonlocal state_expr

		log.info(f"@state_trigger {state_expr}")

		@state_trigger(state_expr)
		def test_state_inner(*args, **kwargs):
			return funcxx(*args, **kwargs)

		return test_state_inner

	return test_state_inner_dec

@test_state('input_boolean.test_1','on')
def do_decorated_toggle():
	log.info('doing toggle from do_decorated_toggle')
	input_boolean.test_2.toggle()
	log.info('done with do_decorated_toggle')

Error:

homeassistant    | 2020-12-24 05:49:51 DEBUG (MainThread) [custom_components.pyscript.eval] file.dec_action.test_state_inner: calling funcxx(, {'trigger_type': 'state', 'var_name': 'input_boolean.test_1', 'value': 'on', 'old_value': 'off', 'context': Context(user_id='64adcd4e4b6b4ad2a5e42990befe70da', parent_id=None, id='704bc285536e75980475ed507ece38f2')})
homeassistant    | 2020-12-24 05:49:51 ERROR (MainThread) [custom_components.pyscript.file.dec_action.test_state_inner] Exception in <file.dec_action.test_state_inner> line 37:
homeassistant    |     			return funcxx(*args, **kwargs)
homeassistant    |                       ^
homeassistant    | TypeError: call() missing 1 required positional argument: 'ast_ctx'

The trouble is that the funcxx that test_state_inner_dec is getting is the actual call method from EvalFunc. This expects ast_ctx as the first parameter which, of course, the pyscript method doesn't have to pass, and even if it did, it would be cumbersome for the user to deal with.

So, instead of passing call in, we need to pass something that doesn't require ast_ctx, and will add it back on itself when called.

I'll see if I can work out a cleaner way to do that starting with a much better base than I had before.

THANK YOU!

[dlashua]

like the above test_state decorator, when @state_trigger (and I assume other built-in decorators) is used in the decorator, the function being called (funcxx in test_state_inner) doesn't get passed ast_ctx. However, when @state_trigger isn't included, then it does get passed (like in your repeat decorator, or in this log_dec decorator, below):

def log_dec(note):
	log.info(f"log_dec init with {note}")

	def log_dec_inner_dec(funcyy):
		nonlocal note

		def log_dec_inner(*args, **kwargs):
			nonlocal note
			log.info(f"NOTE: {note}")

			return funcyy(*args, **kwargs)

		return log_dec_inner

	return log_dec_inner_dec

I think it would be best if log_dec_inner didn't receive ast_ctx (since that's not very useful inside of a user defined pyscript function). But, I'm not sure how to clean that up. make_dec_call (the hacky bit) cleans off the AstEval if its present, which works, but, doesn't seem like the right thing to do.

There must be a better way to just not have ast_ctx there at all, I just can't seem to find it.

[dlashua]

Put another way to make sure I'm explaining it correctly, here's a use of your repeat decorator with a small modification to add logging:

def repeat(num_times):
	def decorator_repeat(func):
		nonlocal num_times
		def wrapper_repeat(*args, **kwargs):
			log.info(f"wrapper_repeat with {args} {kwargs}")
			for _ in range(num_times):
				value = func(*args, **kwargs)
			return value
		return wrapper_repeat
	return decorator_repeat

@time_trigger('startup')
def startup():
	log_test()

@repeat(3)
def log_test():
	log.info('test')

Running the above without my hacky bit added in, you'll notice from the logs, wrapper_repeat gets an AstEval as it's first argument which it then passes to func. And since func (which is just EvalFunc.call), expects to see it, it works fine even though it's weird to have AstEval there and a pyscript user doesn't likely need to have it.

@state_trigger however, expects a function that DOESN'T take AstEval as it's first argument. So, if we try to wrap func with @state_trigger it breaks.

Ideally, func (in your decorator) would NOT expect AstEval as the first argument, and wrapper_repeat wouldn't receive AstEval as the first argument. That way, all the arguments and every call being performed would look just like the functions pyscript users define for @state_trigger, where AstEval is not something they ever see or deal with.

[craigbarratt]

Yes, it doesn't work without your function that strips off any leading ast context and ensures there is one when the function is called. I find it difficult to understand exactly why that's necessary. It seems vaguely like the movie Inception. I need to think about this some more...

[dlashua]

Yes, exactly! I've tried all manner of inspection inside of eval_decorators, but, it's still a mystery. @state_trigger handles the function perfectly and it uses ast_ctx.call_func() to invoke. So modification there is likely not needed. Hopefully, you can figure it out, because my brain is fried on it. lol.

[craigbarratt]

I think it will be cleaner to move the decorator calls up into ast_functiondef using the EvalFunc object that gets created there to call the decorators, rather than overwriting the call method.

I'll merge this PR and edit from there.

[dlashua]

I'm still looking over the code to figure out exactly what you did. But all of my tests are working.

I've tested...

a decorator function with no arguments.
a decorator function with arguments (like your repeat above)
a class based decorator function with no arguments
a class based decorator function with arguments
all of the above decorators with a @state_trigger inside of them

everything seems to work.

now, to go figure out what you did and then start converting some of my "production" code to see if any bugs crop up.

This is fantastic! Thank you!