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

Dependency solver #121

Merged
merged 14 commits into from
Dec 19, 2022
Merged

Dependency solver #121

merged 14 commits into from
Dec 19, 2022

Conversation

da-h
Copy link
Owner

@da-h da-h commented Oct 18, 2022

Graph-Based Dependency Evaluation

Attention: This is a breaking change.

This MR changes the way dependencies are defined, checked & evaluated.

Description

Previous implementation for dependency solving has been done for very simple cases, where no loops or harder dependencies could be parsed.
This MR changes that by refactoring the whole registration process and the evaluation of the variables.

Breaking API-Changes

  • Previously, we would allow any lambda expression of the form lambda state, event: ... to be used for state-registrations.
    **This PR changes the way lambda expressions are parsed and evaluated.
  • The new dependency-system makes mf.like unnecessary, thus mf.like has been removed from miniflask completely

New Behavior

This MR implements graph based dependencies.
To accomplish this, it does the following during registration:

  1. New API: mf.state_registrations contains all information of all variable registrations.
    • It's keys are the global variable names,
    • the values represent the variable registrations. In more detail the values are lists of state_node-instances. This new class wraps all the information needed for graph-based dependency evaluation and checks without ever saving the actual data. This is to ensure also large amounts of data to be saved in state-variables without every worrying about unintentional copies in the interpreter.
    • Internally, the code of the lambda-expression is parsed. I am sure there could be a better way to achieve this, i.e. by using a mocking library or a ast-representation of the functions to parse these. As this is the easiest way, I have implemented the source-code parsing. Though, I am open to work on a more elaborate way of achieving the same effect in the future. See below for a short discussion on this.
  2. Note: The behavior of variable-overwrites and the order of variable definitions is still the same.
    (I have taken special care to not change this).
  3. After all modules have been loaded, the cli arguments are typically evaluated. This evaluation consists now of the following steps:
    a. First we look for the last registration for every variable, taking the usual order (see 2.) into account.
    b. Next we check the graph for circular dependencies or unresolved dependencies (using a simple DFS-routine). In case something is found, all errors are shown at once.
    c. Using the same routine from (b.), we sort all variables into an order that can be parsed linearly.
    d. Next, we evaluate all variables for the first time taking their dependencies into account.
    e. Then we can define the cli-argument parser using the values of the first pass (d.).
    f. Finally, we match the cli-arguments to the actual state-variables (unchanged) and re-evaluate the graph to take overwrites into account.

Summary / Things done in this MR

  • Implementing graph-based state-dependencies
  • Removing the mf.like-API from miniflask.
  • Adding tests for the new API, placed in tests/state/dependencies
  • Adapted [settings] to match the new API
  • Fixed a test case that behaved differently with the old evaluation system, namely when changes to variables actually to apply. The new behavior is to do that directly.
    The test checked for values before the mf.run, i.e. the first event that is called. The new implementation does not require to "remember" the initial state until the user-parameters are read from cli, which was just a work-around in the old system.

Check all before creating this PR:

  • Documentation adapted
  • unit tests adapted / created

Examples

The following list shows all types of lambda expressions supported with this MR**:

lambda: somefunction("arguments")
lambda state: state["myvariable"]
lambda state: (state["myvariable"] * 5) + state["othervariable"]
lambda state: somefunction(state["myvariable"]) + state["othervariable"]
lambda state: state["myvariable"] * 5 if "myvariable" in state else state["othervariable"]
lambda state: state["myvariable"] * state["othervariable"] if "myvariable" in state and "othervariable" in state else state["yetanothervariable"]

Notes on Lambda-Parsing

Checking for state-calls is rather reliable in one-liners. The harder part is to find out which variables can be omitted, because the user wishes to have one variable out of a list.
I have solved this here by specifically allowing lambda expressions of a predefined form,

anyexpression(state[x]) if x in state else otherexpression(state[y])

allowing also multiple variables to be used, i.e. cond1 and cond2 and cond3 and ....
All this is mainly to minimize the dependencies.
In case the source-code matching of the if-else-expression above fails, all state-calls will be used as a dependency.

@da-h da-h requested a review from sbrodehl October 18, 2022 14:47
@da-h da-h force-pushed the dependency_solver branch 2 times, most recently from 3c2178a to 2b024e5 Compare October 18, 2022 15:17
@da-h da-h force-pushed the dependency_solver branch from 2b024e5 to 6a8c67f Compare October 18, 2022 15:20
@da-h da-h marked this pull request as ready for review October 18, 2022 15:21
Copy link
Collaborator

@sbrodehl sbrodehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR , it looks like a very nice idea!

For now, I have two (more general) comments:

  • DFS: Should the recursive DFS be improved right away?
  • lambda expression: Should the AST be parsed and checked, instead of manual search?

self.fn = fn
self.fn_src = getsource(self.fn) if self.fn is not None else None

if self.fn_src is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the code checked manually, instead of parsing the complete expression and creating the AST, looking for variable usage?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If have implemented the dependency checking via the python AST, see miniflask/dev-ast branch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge #122, then we can merge this afterwards?

@da-h da-h force-pushed the dependency_solver branch 3 times, most recently from 10c0a03 to 205f07c Compare October 21, 2022 12:42
sbrodehl and others added 4 commits December 19, 2022 16:15
Fix order of deps, now always sorted.

Add more test cases.

Remove unused argument 'state'.

tests: Add test for failing stuff.

Split from "Remove 'meta' inspection."
Inspect register() call to maybe get more insights.

Remove 'meta' inspection.

Ensure single expression is found in source.

Define and use private members.

Fix string search for 'state' argument.

Remove not allowed statements.

Remove unused import.

Remove empty lines.

Add noqa to class.

Spam noqa.

fjgruiewhjfiogbhpa

fjgruiewhjfiogbhpa

Disable more warnings.

Fix typo.
@da-h
Copy link
Owner Author

da-h commented Dec 19, 2022

Great work here, @sbrodehl.

Thanks a lot for helping me out for this tough implementation! 🚀

@da-h da-h merged commit 513832b into v5-dev Dec 19, 2022
@da-h da-h deleted the dependency_solver branch December 19, 2022 15:22
@da-h da-h mentioned this pull request Dec 20, 2022
2 tasks
@da-h da-h mentioned this pull request Jan 9, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants