jruizgit / rules

Durable Rules Engine
MIT License
1.15k stars 206 forks source link

get_facts() not seeing asserted facts from a consequent #350

Open mpettis opened 4 years ago

mpettis commented 4 years ago

Linked to this issue: https://github.com/jruizgit/rules/issues/307

It looks like get_facts() is not getting facts asserted in a consequent. I don't know if it is failing -- I get no error message, but the last "Third" print statement is not executed -- possibly a hint.

from durable.lang import *

with ruleset('test'):
    @when_all(m.subject == 'hello')
    def compute_thrashing(c):
        # consequent
        print("First")
        newdict = {"fact_type": "new"}
        print("Second")
        c.assert_fact(newdict)
        print("Third")

assert_fact('test', {"subject":"hello"})

print(get_facts('test'))

Result from running as script:

First
Second
[{'subject': 'hello'}]
reinoldus commented 4 years ago

when you add the following rule the fact gets added:

@when_all(+m.fact_type)
    def end(c):
        print("end")

I don't know why. You could put what you need into the state like so: c.s.fact_type = "new". Then you would see it in get_state

trankos commented 4 years ago

c.assert_fact(newdict) fails because of the fact ({"fact_type": "new"}) is not being observed, that is, there is no rule triggered by it. That's why when you added a new rule triggered by this kind of fact (@when_all(+m.fact_type)) the fact is asserted. Besides, exceptions raised inside a consequent are captured and not reraised. In this case, assert_fact fails silently.

mpettis commented 4 years ago

Thank you both @reinoldus and @trankos ! To @trankos point -- that means that c.assert_fact(new_dict) does not trigger anything? I was assuming that asserting a fact and triggering a rule were the same thing -- but it isn't? What is the difference between asserting a fact and triggering a rule? Because I assumed that one outcome of a triggered rule was asserting a fact, and you could update the facts with just assert_fact.

trankos commented 4 years ago

As far as I know, assert_fact triggers the rule resolution process, that is, check if exists any rule, or rules, that can be triggered because of that fact (its antecedent is true) and, if any, execute its consequent. But it is possible that no rule be triggered if no rule antecedent is true.

mpettis commented 4 years ago

That all makes sense to me. The model in my mind though is that even if I asserted a fact that does not trigger any rule, that fact still exists in the ruleset. So if I do a get_facts() on the ruleset after asserting a fact that did not trigger any rules, I should still see that fact (that didn't trigger any rules) returned. Am I missing something conceptually here?

trankos commented 4 years ago

Yes, that's how it works. But, coming back to your first comment, the fact only stays, if and only if it's observed and that means that can match some rule antecedent. In other case, the fact is ignored. This behavior is explained here. IMHO, for the sake of simplicity and better performance, @jruizgit has taken some design decisions like this. In other inference engines, is mandatory define explicitly fact structure. @jruizgit approach is simply and clever: rule antecedent defines implicitly the structure of a fact, but only partially. A fact has to content, at least, a minimum of properties to be considered a "proper fact" and that properties are those that appear in rules antecedents, but could have more properties.

mpettis commented 4 years ago

Thanks @trankos , this is clearing some things up. Does the following make sense:

from durable.lang import *

with ruleset('test'):
    @when_all(m.subject == 'exists')
    def myassert(c):
        c.assert_fact({"fact_type": "new"})

assert_fact('test', {"subject":"exists"})
assert_fact('test', {"subject":"nonexistent"})

Returns an error, because the globally asserted fact does not match any rule antecedants:

C:\Users\IRINZN\tmp\durable_rules>python github-issue-350.py
Traceback (most recent call last):
  File "github-issue-350.py", line 10, in <module>
    assert_fact('test', {"subject":"nonexistent"})
  File "C:\Users\IRINZN\anaconda3\lib\site-packages\durable\lang.py", line 676, in assert_fact
    return get_host().assert_fact(ruleset_name, fact, complete)
  File "C:\Users\IRINZN\anaconda3\lib\site-packages\durable\engine.py", line 817, in assert_fact
    return self._handle_function(rules, rules.assert_fact, fact, complete)
  File "C:\Users\IRINZN\anaconda3\lib\site-packages\durable\engine.py", line 790, in _handle_function
    rules.do_actions(func(args), callback)
  File "C:\Users\IRINZN\anaconda3\lib\site-packages\durable\engine.py", line 343, in assert_fact
    return self._handle_result(durable_rules_engine.assert_fact(self._handle, json.dumps(fact, ensure_ascii=False)), fact)
  File "C:\Users\IRINZN\anaconda3\lib\site-packages\durable\engine.py", line 328, in _handle_result
    raise MessageNotHandledException(message)
durable.engine.MessageNotHandledException: {'subject': 'nonexistent'}

This worries me a bit because if I assert some facts that don't trigger any rules, it will give me an error. It may optimize some things, but it seems like a potential stumbling block. I can envision common use cases where I assert a bunch of facts, but only some of them trigger rules.

I could see myself adding a key/value pair to every fact called something like 'type':'fact' and then having a rule that just found all facts that are of 'type':'fact' and then doing nothing with that result just to avoid triggering this error... like so:

from durable.lang import *

with ruleset('test'):
    @when_all(m.subject == 'exists')
    def myassert(c):
        c.assert_fact({"subject":"new insert", "type":"fact"})
    @when_all(m.subject == 'rule not fired')
    def myassert_nofire(c):
        c.assert_fact({"subject":"rule did not fire", "type":"fact"})
    @when_all(m.type == 'fact')
    def noop(c):
        pass

assert_fact('test', {"subject":"exists", "type":"fact"})
assert_fact('test', {"subject":"nonexistent", "type":"fact"})

print(get_facts('test'))

This works with no error, and all of the asserted fact, global or derived, do show up:

C:\Users\IRINZN\tmp\durable_rules>python github-issue-350.py
[{'subject': 'new', 'type': 'fact', 'sid': '0'}, {'subject': 'nonexistent', 'type': 'fact'}, {'subject': 'exists', 'type': 'fact'}]

But this kind of seems like a hack on my part, to be honest...

trankos commented 4 years ago

Indeed, it also seems to me that it is the only way to add a fact to the knowledge base (that can be used in the future). For example, a rule like this:

   @when_all(+m.subject)
    def catch_all(c):
        pass

will catch every fact with a property named subject, and will be stored into knowledge base for future matches.

Is good or bad design decision to force a rule match to store a fact into knowledge base? Yes, it smells like a hack but:

Thank you, @mpettis, that has been a good point. I'm a newbie here and I wasn't sure about the better way to store facts but this discussion clear my mind.

mpettis commented 4 years ago

I'm a newbie to rules myself, so this has been a very helpful discussion for me as well!