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

Add a clear method in Facts API #250

Closed
R-Gerard opened this issue Jan 9, 2020 · 6 comments
Closed

Add a clear method in Facts API #250

R-Gerard opened this issue Jan 9, 2020 · 6 comments
Labels
Milestone

Comments

@R-Gerard
Copy link

R-Gerard commented Jan 9, 2020

Is there a recommended way for a Rule to halt the firing process of InferenceRulesEngine?

Currently I'm clearing out the Facts map with .asMap().clear() but this breaks encapsulation and the law of Demeter, so it isn't a satisfactory solution.

Example:

@Rule(name = "early_exit_rule", description = "Halt the InferenceRulesEngine early if we have sufficient info", priority = 99)
public class EarlyExitRule {

    private final Facts facts;

    public EarlyExitRule(Facts facts) {
        this.facts = facts;
    }

    @Condition
    public boolean when(@Fact("should_exit_early") boolean shouldExitEarly) {
        return shouldExitEarly;
    }

    @Action
    public void then() throws Exception {
        System.out.println("should_exit_early == true -> halt the InferenceRulesEngine");
        // Clear out all other facts to early-exit the inference process
        facts.asMap().clear();
    }
}
@fmbenhassine
Copy link
Member

Inference rules engines are not designed to be halted. The idea is to continuously select and run candidate rules until no more rules are applicable. So trying to halt the engine in the middle of the inference process goes against the goal in the first place.

I believe a properly configured DefaultRulesEngine is more appropriate for this use case. For example, I see the priority of EarlyExitRule is set to 99, so your can set rulePriorityThreshold to 98 for example to halt the engine when it reaches this rule.

Do you agree?

@R-Gerard
Copy link
Author

R-Gerard commented Mar 9, 2020

I believe a properly configured DefaultRulesEngine is more appropriate for this use case.

Actually I do want to use the inference engine in this case. The default rules engine applies rules to facts once and only once. I want to be able to repeatedly apply rules to facts in order to generate more facts and fire rules again as necessary, but only until a goal has been reached. I'm able to do that with my toy example above, but it's a hacky solution.

@fmbenhassine
Copy link
Member

fmbenhassine commented Mar 15, 2020

it's fine to use the inference engine if you need it, you can still use the rulePriorityThreshold parameter to stop execution at your EarlyExitRule. Have you tried that?

BTW, I see nothing wrong with clearing facts to halt the execution. By design, a rule's action is not a function in this type of systems, it is an action that might have a side effect on facts: it can add, modify or remove facts to drive the execution flow of subsequent rules. So according to your statement:

I want to be able to repeatedly apply rules to facts in order to generate more facts and fire rules again as necessary, but only until a goal has been reached

do your rules remove facts at some point? I don't know the details of your use case, but:

  • if your rules do remove facts, I think your goal should be designed as: "your current goal" or "no more facts".
  • Otherwise, clearing out the facts in your "exit" rule is ok IMO.

wdyt?

@R-Gerard
Copy link
Author

Yes, I agree, but directly manipulating the underlying map in the Facts object feels hacky to me (hence my original question). It breaks OO encapsulation. I would expect asMap to return an ImmutableMap copy in an ideal world, or only a KeySet.

How do you feel about adding a clear method to Facts, or halt to InferenceRulesEngine (or both)?

@fmbenhassine
Copy link
Member

directly manipulating the underlying map in the Facts object feels hacky to me (hence my original question). It breaks OO encapsulation. I would expect asMap to return an ImmutableMap copy in an ideal world, or only a KeySet.

I agree, and I consider this as a bug so I opened #267 (same for Facts#iterator for which I opened #268).

How do you feel about adding a clear method to Facts, or halt to InferenceRulesEngine (or both)?

I already tried to introduce a halt method (see e2e4cef) but the problem is that the call to fire is blocking:

rulesEngine.fire(rules, facts); // blocking call here
rulesEngine.halt(); // too late when reached..

Do you see? It is possible to launch the rules engine in a different thread and halt it from another thread, but I don't like this idea because it forces me to run the engine asynchronously to be able to use a "feature" (I'm a firm believer in "Leave concurrency to the caller" from the the zen of go).

I think adding a clear method to Facts + fixing #267 is a good start for now, because as I mentioned, an action might clear facts by design for this kind of systems. Do you agree? If yes, I can proceed and add the method in the upcoming release.

@R-Gerard
Copy link
Author

R-Gerard commented Apr 3, 2020

Ah, that makes perfect sense. I agree that adding a clear method to Facts + fixing #267 is a good start for now.

@fmbenhassine fmbenhassine changed the title Halt InferenceRulesEngine using Rule Add a clear method to Facts Apr 4, 2020
@fmbenhassine fmbenhassine modified the milestones: 3.5.0, 4.0.0 Apr 4, 2020
@fmbenhassine fmbenhassine changed the title Add a clear method to Facts Add a clear method in Facts API Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants