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

Lambda functions can be defined in config #374

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JonathanDHarris
Copy link
Contributor

@JonathanDHarris JonathanDHarris commented Nov 20, 2020

Description

Allow users to define lambda functions in their configs. Resolve's #318

Checklist

  • Follow the Contributor Guidelines
  • Write unit tests
  • Write documentation strings
  • Assign someone from your working team to review this pull request
  • Assign someone from the infrastructure team to review this pull request

@JonathanDHarris JonathanDHarris requested a review from a team as a code owner November 20, 2020 12:06
@ntessore
Copy link
Member

ntessore commented Nov 20, 2020

I have a very similar changeset on my machine, which I never propose because of the dangers of eval(). My idea was to only eval the lambda function body:

fdef = 'x**2 + 3'
f = lambda x: eval(fdef, locals())

I think this would severely limit the harm that can be done in the lambda function body. But then I never figured out a good way to specify the function arguments.

@JonathanDHarris
Copy link
Contributor Author

JonathanDHarris commented Nov 20, 2020

I'm not sure there's anyway to resolve #318 without using eval, so I think a decision needs to be made to use eval or close the issue off as won't fix.

e: just saw your edit, we can go with that if you prefer it. Can you explain how it limits the damage eval can cause? My understanding is the user could still put any arbitrary code in their config and execute it.

@ntessore
Copy link
Member

ntessore commented Nov 20, 2020

It's true (at the moment) that arbitrary functions can be run from config files, and that's another issue. But I wasn't thinking of malicious code rm -rfing the system as much as someone doing something weird that messes up SkyPy internals. Hence why I would prefer to limit the input to be only a single statement.

There is a way to build lambdas programmatically, but it requires work:

>>> lambda_args = ['x', 'y']
>>> lambda_body = 'x**2 + y + 1'
>>>
>>> import ast
>>> args = ast.arguments(posonlyargs=[],
...                      args=[ast.arg(arg) for arg in lambda_args],
...                      kwonlyargs=[],
...                      kw_defaults=[],
...                      defaults=[])
>>> body = ast.parse(lambda_body, mode='eval').body
>>> expr = ast.Expression(ast.Lambda(args, body))
>>> expr.line_no = 1  # from YAML
>>> expr.col_offset = 1  # from YAML
>>> expr = ast.fix_missing_locations(expr)
>>> code = compile(expr, filename='<ast>', mode='eval')
>>> func = eval(code, {})  # lambda works in a vacuum
>>> func(2, 3)
8

And this still leaves the problem of how to obtain the list of arguments and the function body from the YAML.

Edit for your edit about my edit: If the eval() is the body of the lambda, then I believe it can only contain a single expression which cannot be an import statements and such.

@JonathanDHarris
Copy link
Contributor Author

"And this still leaves the problem of how to obtain the list of arguments and the function body from the YAML."

Something like this should work

function_def: !lambda
  x: 1
  b: 2
  function_def:'x**2 + y + 1'

@ntessore
Copy link
Member

We want the lambdas to produce callables and not values, in order to define parametrisations such as in the examples in a flexible manner.

Since there is no good way to use YAML intrinsics to make this easier, I think the !lambda tag should simply take its value as a scalar node (string) and parse. If we are using Python lambda syntax:

mycallable: !lambda 'x, y: x**2 + y + 1'

Since there's a : in the syntax, we have no way around quotes. However, I have always been fond of Julia's anonymous function syntax:

mycallable: !lambda (x, y) -> x**2 + y + 1

In addition to all of this, we will also need to export symbols such as exp, sin, cos, etc. for the evaluation of the function body. That's quite the big task, so don't feel obligated to tackle this issue.

@ntessore
Copy link
Member

On further thought there's even a whole other infrastructure dimension to this, because the lambda's should have access to certain other parts of the pipeline, such as parameters. @JonathanDHarris I will review this now and then we will iterate later on.

skypy/pipeline/_config.py Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Tessore <n.tessore@gmail.com>
ntessore
ntessore previously approved these changes Nov 21, 2020
@rrjbca rrjbca changed the base branch from master to main February 22, 2021 13:39
@rrjbca rrjbca dismissed ntessore’s stale review February 22, 2021 13:39

The base branch was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants