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

Strange NPE issue #500

Open
victorrodrigueznadq opened this issue Jun 11, 2024 · 16 comments
Open

Strange NPE issue #500

victorrodrigueznadq opened this issue Jun 11, 2024 · 16 comments

Comments

@victorrodrigueznadq
Copy link

This is my original clara rule, which was converted from a drools implementation:

(defrule add-all-dlp-firm-symbols
  "Add All DLP Firm Symbols"
  {:salience 1000000}
  [?billingDates <- BillingDates
   ]
  [?dlpFirm <- DlpFirm
   (= ?mpid mpid )
   (= ?symbol symbol )
   ]
  [?symbolBillingLink <- SymbolBillingProgramLink
   (= accountId ?mpid)
   (= symbol ?symbol)
   (nil? terminateDate)
   ]
  =>
  (def volumeAccumulation (VolumeAccumulation.))
  (.setAccountIdentifier volumeAccumulation ?mpid)
  (.setSymbol volumeAccumulation ?symbol)
  (.setNumberOfDays volumeAccumulation (.getNumberOfTradingDates ?billingDates))
  (.setTotalVolume volumeAccumulation (long 0))
  (.setAccumulationLevel volumeAccumulation VolumeAccumulation$AccumulationLevel/ACCOUNT_SYMBOL)
  (.setAccumulationCode volumeAccumulation "nqeAddDlp")
  (.setFirmIdentifier volumeAccumulation (.getFirmId ?symbolBillingLink))
  (insert! (Map/of "$volumeAccumulationWriter" volumeAccumulation))
  )

I was having a memory leak because we didn't have a check to prevent the same VolumeAccumulation records from being created and inserted over an over again each time the rules were fired. So then I added a :not check. I kept betting NPE exceptions complaining about firmIdentifier being null, so I kept adding more and more (some?) checks to ensure, supposedly, that that couldn't never happen, but thusfar, I've been unable to erradicate that NPE exception. Here's what the rule currently looks like (added (insert! volumeAccumulation) to the end of the RHS):

(defrule add-all-dlp-firm-symbols
  "Add All DLP Firm Symbols"
  {:salience 1000000}
  [?billingDates <- BillingDates
   ]
  [?dlpFirm <- DlpFirm
   (= ?mpid mpid )
   (= ?symbol symbol )
   ]
  [?symbolBillingLink <- SymbolBillingProgramLink
   (= accountId ?mpid)
   (= symbol ?symbol)
   (nil? terminateDate)
   (some? firmId)
   ]
  [:not [VolumeAccumulation
         (some? numberOfDays)
         (some? firmIdentifier)
         (some? totalVolume)
         (= accountIdentifier ?mpid)
         (= symbol ?symbol)
         (= numberOfDays (.getNumberOfTradingDates ?billingDates))
         (= totalVolume (long 0))
         (= accumulationLevel VolumeAccumulation$AccumulationLevel/ACCOUNT_SYMBOL)
         (= accumulationCode "nqeAddDlp")
         (= firmIdentifier (.getFirmId ?symbolBillingLink))
         ]]
  =>
  (def volumeAccumulation (VolumeAccumulation.))
  (.setAccountIdentifier volumeAccumulation ?mpid)
  (.setSymbol volumeAccumulation ?symbol)
  (.setNumberOfDays volumeAccumulation (.getNumberOfTradingDates ?billingDates))
  (.setTotalVolume volumeAccumulation (long 0))
  (.setAccumulationLevel volumeAccumulation VolumeAccumulation$AccumulationLevel/ACCOUNT_SYMBOL)
  (.setAccumulationCode volumeAccumulation "nqeAddDlp")
  (.setFirmIdentifier volumeAccumulation (.getFirmId ?symbolBillingLink))
  (insert! volumeAccumulation)
  (insert! (Map/of "$volumeAccumulationWriter" volumeAccumulation))
  )

And, here's the exception:

Caused by: java.lang.NullPointerException: Cannot invoke "java.lang.Integer.intValue()" because "this.firmIdentifier" is null
	at com.nasdaq.edm.rms.accumulation.VolumeAccumulation.getFirmIdentifier(VolumeAccumulation.java:252)
	at clojure.core$eval388$AN_136_AE__507.invoke(NO_SOURCE_FILE:42)
	at clara.rules.engine$alpha_node_matches$iter__4093__4097$fn__4098$fn__4099$fn__4100.invoke(engine.cljc:517)
	... 28 common frames omitted

Any ideas what could possibly be going on?

@victorrodrigueznadq
Copy link
Author

victorrodrigueznadq commented Jun 13, 2024

@EthanEChristian we made a change in our java code that seems to have resolved the NPE issue, but now we have an infinite loop of this rule firing after the (insert! volumeAccumulation) even though the :not is meant to prevent this rule from firing if we've already inserted a volume accumulation record for the given DlpFirm and SymbolBillingProgramLink. Thoughts? I added some logging and here's the output:

08:19:19.081 [main] INFO com.nasdaq.rms.rules.nqe.transactional.accumulations.nqe_rms_accumulation_rules_xlsx -- Inserted: com.nasdaq.edm.rms.accumulation.VolumeAccumulation@424a6d8c[accountIdentifier=X,accumulationCode=nqeAddDlp,accumulationLevel=ACCOUNT_SYMBOL,avgDailyVolume=<null>,capInformation=<null>,capLevelId=<null>,capReached=false,firmIdentifier=12345,numberOfDays=19,portIdentifier=<null>,symbol=X,totalVolume=0]
08:19:19.081 [main] INFO com.nasdaq.rms.rules.nqe.transactional.accumulations.nqe_rms_accumulation_rules_xlsx -- Inserted: com.nasdaq.edm.rms.accumulation.VolumeAccumulation@66e4989a[accountIdentifier=X,accumulationCode=nqeAddDlp,accumulationLevel=ACCOUNT_SYMBOL,avgDailyVolume=<null>,capInformation=<null>,capLevelId=<null>,capReached=false,firmIdentifier=12345,numberOfDays=19,portIdentifier=<null>,symbol=X,totalVolume=0]
08:19:19.081 [main] INFO com.nasdaq.rms.rules.nqe.transactional.accumulations.nqe_rms_accumulation_rules_xlsx -- Inserted: com.nasdaq.edm.rms.accumulation.VolumeAccumulation@79c0306c[accountIdentifier=X,accumulationCode=nqeAddDlp,accumulationLevel=ACCOUNT_SYMBOL,avgDailyVolume=<null>,capInformation=<null>,capLevelId=<null>,capReached=false,firmIdentifier=12345,numberOfDays=19,portIdentifier=<null>,symbol=X,totalVolume=0]

I guess this will eventually lead to another memory leak

@mrrodriguez
Copy link
Collaborator

mrrodriguez commented Jun 13, 2024

@EthanEChristian we made a change in our java code that seems to have resolved the NPE issue, but now we have an infinite loop of this rule firing after the (insert! volumeAccumulation) even though the :not is meant to prevent this rule from firing if we've already inserted a volume accumulation record for the given DlpFirm and SymbolBillingProgramLink. Thoughts? I added some logging and here's the output:

08:19:19.081 [main] INFO com.nasdaq.rms.rules.nqe.transactional.accumulations.nqe_rms_accumulation_rules_xlsx -- Inserted: com.nasdaq.edm.rms.accumulation.VolumeAccumulation@424a6d8c[accountIdentifier=X,accumulationCode=nqeAddDlp,accumulationLevel=ACCOUNT_SYMBOL,avgDailyVolume=<null>,capInformation=<null>,capLevelId=<null>,capReached=false,firmIdentifier=12345,numberOfDays=19,portIdentifier=<null>,symbol=X,totalVolume=0]
08:19:19.081 [main] INFO com.nasdaq.rms.rules.nqe.transactional.accumulations.nqe_rms_accumulation_rules_xlsx -- Inserted: com.nasdaq.edm.rms.accumulation.VolumeAccumulation@66e4989a[accountIdentifier=X,accumulationCode=nqeAddDlp,accumulationLevel=ACCOUNT_SYMBOL,avgDailyVolume=<null>,capInformation=<null>,capLevelId=<null>,capReached=false,firmIdentifier=12345,numberOfDays=19,portIdentifier=<null>,symbol=X,totalVolume=0]
08:19:19.081 [main] INFO com.nasdaq.rms.rules.nqe.transactional.accumulations.nqe_rms_accumulation_rules_xlsx -- Inserted: com.nasdaq.edm.rms.accumulation.VolumeAccumulation@79c0306c[accountIdentifier=X,accumulationCode=nqeAddDlp,accumulationLevel=ACCOUNT_SYMBOL,avgDailyVolume=<null>,capInformation=<null>,capLevelId=<null>,capReached=false,firmIdentifier=12345,numberOfDays=19,portIdentifier=<null>,symbol=X,totalVolume=0]

I guess this will eventually lead to another memory leak

@victorrodrigueznadq

Seems like you may have created a "logical loop" in with regards to the truth maintenance system (aka. TMS).

There is a directly related gist of mine here on the logical loop situation.

This post I wrote a while back discusses something similar if the problem becomes more of a "logical update" to facts: https://www.metasimple.org/2017/12/23/clara-updating-facts.html

@victorrodrigueznadq
Copy link
Author

@mrrodriguez thanks "tocayo"

@victorrodrigueznadq
Copy link
Author

victorrodrigueznadq commented Jun 17, 2024

Still struggling here @EthanEChristian @mrrodriguez ....decided to take a different approach....

Rather than creating the VolumeAccumulation facts from clara, now I'm inserting them (364 of them) from the calling java code, before calling fireRules. And now my rule looks like this:

(defrule add-all-dlp-firm-symbols
  "Add All DLP Firm Symbols"
  {:salience 1000000}
  [?volumeAccumulation <- VolumeAccumulation
   (= accumulationCode "nqeAddDlp")]
  =>
  (.info lRuleLogger (str "dlpCounter: " (swap! dlpCounter inc)))
  (insert-unconditional! (Map/of "$volumeAccumulationWriter" ?volumeAccumulation))
  (retract! ?volumeAccumulation)
  )

However, it seems as if the retract isn't actually working because I'll see a burst of 364 log statements with every subsequent call to fireRules from further processing the java code does. I'm only expecting to see 364 log statements adn nothing more since each fact that is handled should be retracted, and should never need to be processed again.

Thoughts?

@mrrodriguez
Copy link
Collaborator

Still struggling here @EthanEChristian @mrrodriguez ....decided to take a different approach....

Rather than creating the VolumeAccumulation facts from clara, now I'm inserting them (364 of them) from the calling java code, before calling fireRules. And now my rule looks like this:

(defrule add-all-dlp-firm-symbols
  "Add All DLP Firm Symbols"
  {:salience 1000000}
  [?volumeAccumulation <- VolumeAccumulation
   (= accumulationCode "nqeAddDlp")]
  =>
  (.info lRuleLogger (str "dlpCounter: " (swap! dlpCounter inc)))
  (insert-unconditional! (Map/of "$volumeAccumulationWriter" ?volumeAccumulation))
  (retract! ?volumeAccumulation)
  )

However, it seems as if the retract isn't actually working because I'll see a burst of 364 log statements with every subsequent call to fireRules from further processing the java code does. I'm only expecting to see 364 log statements adn nothing more since each fact that is handled should be retracted, and should never need to be processed again.

Thoughts?

@victorrodrigueznadq I understand the problem statement here. I think I need to see a bit more about your external process though. Could you show at least an example of the sequence of calls you are making from one fireRules to the next, also including how you are inserting the initial facts into the session.

@victorrodrigueznadq
Copy link
Author

victorrodrigueznadq commented Jun 21, 2024

Still struggling here @EthanEChristian @mrrodriguez ....decided to take a different approach....
Rather than creating the VolumeAccumulation facts from clara, now I'm inserting them (364 of them) from the calling java code, before calling fireRules. And now my rule looks like this:

(defrule add-all-dlp-firm-symbols
  "Add All DLP Firm Symbols"
  {:salience 1000000}
  [?volumeAccumulation <- VolumeAccumulation
   (= accumulationCode "nqeAddDlp")]
  =>
  (.info lRuleLogger (str "dlpCounter: " (swap! dlpCounter inc)))
  (insert-unconditional! (Map/of "$volumeAccumulationWriter" ?volumeAccumulation))
  (retract! ?volumeAccumulation)
  )

However, it seems as if the retract isn't actually working because I'll see a burst of 364 log statements with every subsequent call to fireRules from further processing the java code does. I'm only expecting to see 364 log statements adn nothing more since each fact that is handled should be retracted, and should never need to be processed again.
Thoughts?

@victorrodrigueznadq I understand the problem statement here. I think I need to see a bit more about your external process though. Could you show at least an example of the sequence of calls you are making from one fireRules to the next, also including how you are inserting the initial facts into the session.

Sure @mrrodriguez ....creating the session...

WorkingMemory lCREmptySession = null;
try {
	lCREmptySession = RuleLoader.loadRules(lClaraRuleNamespaces);
}
catch (Throwable lTheErr) {
....
}

Inserting some reference data into the session that will persist across latter calls to fireRules...

WorkingMemory lCRGlobalDataProvidersSession = lCREmptySession.insert(
	lGlobalDataProviders.stream().map(globalDataProvider -> {
		return globalDataProvider.getData();
	}).toList());

Adding more reference data, that again will persist across latter calls to fireRules...

WorkingMemory lCRRefDataProvidersSession = lCRGlobalDataProvidersSession;
for (RefDataProvider refDataProvider : dataProviderService.getRefDataProviders(pRulesExecution,
	pBillingRulesConfiguration)) {
lCRRefDataProvidersSession = lCRRefDataProvidersSession.insert(refDataProvider);
}

Creating working memory array from session...

final WorkingMemory lCRExecutingRulesSession[] = new WorkingMemory[] {lCRRefDataProvidersSession};

That session gets passed into a back pressure controller (and assigned to baseClaraRulesSession) that we have and in there we create a worker as follows:

WorkingMemory lWorkingMemory = sessionWorkingMemories.get(lPartitionKeyAndCount.getKey());

ClaraRulesSessionWorker claraRulesSessionWorker =
	new ClaraRulesSessionWorker(lWorkingMemory == null ? baseClaraRulesSession : lWorkingMemory,
			lPartitionKeyAndCount.getKey(),
			lBillableItems, progressMonitor, sessionResultQueryLocation,
			writerQueryResultsQueue);

Then we call the call method on the worker and from there is where fireRules will be called..

CompletableFuture<WorkingMemory> lWorkingMemoryCompletableFuture = CompletableFuture.supplyAsync(() -> {
try {
	WorkingMemory lModifiedWorkingMemory = claraRulesSessionWorker.call();
	sessionWorkingMemories.put(lPartitionKeyAndCount.getKey(), lModifiedWorkingMemory);
	return lModifiedWorkingMemory;
}
catch (Exception theErr) {
	final String lExceptionMsg = String.format(
			"""
					|> ERROR: RulesServiceImpl::BackPressureController::ClaraRulesSessionWorker::call for %s Failed with Exception: %s
					""", lPartitionKeyAndCount.getKey(), theErr.toString());
	throw new RuntimeException(lExceptionMsg, theErr);
}
}, sessionExecutorService);

Inside the call method we loop through the transactions we are processing insert one, call fireRules, and retract it....

for (Object billableItem : billableItems) {
if (billableItem != BILLABLE_ITEM_TERMINATOR) {
	var lBillableItemInAList = List.of(billableItem);
	lClaraRulesSession = lClaraRulesSession.insert(lBillableItemInAList).fireRules();
	// write results to Data Writer Queue
	Iterable<QueryResult> lQueryResults =
			lClaraRulesSession.query(sessionResultQueryLocation);
	for (QueryResult lQueryResult : lQueryResults) {
		writerQueryResultsQueue.add(lQueryResult);
	}
	// reset session for next iteration
	lClaraRulesSession = lClaraRulesSession.retract(lBillableItemInAList);
	lProcessedItemCount++;
	progressMonitor.incrementProcessedItemCount();
}
if (Thread.currentThread().isInterrupted()) {
	throw new InterruptedException("RulesServiceImpl::ClaraRulesSessionWorker::call() - Thread Interrupted");
}
}

That's it.

@victorrodrigueznadq
Copy link
Author

sorry, we insert lists there at the end. could be one or more.

@mrrodriguez
Copy link
Collaborator

mrrodriguez commented Jun 21, 2024

Still struggling here @EthanEChristian @mrrodriguez ....decided to take a different approach....
Rather than creating the VolumeAccumulation facts from clara, now I'm inserting them (364 of them) from the calling java code, before calling fireRules. And now my rule looks like this:

(defrule add-all-dlp-firm-symbols
  "Add All DLP Firm Symbols"
  {:salience 1000000}
  [?volumeAccumulation <- VolumeAccumulation
   (= accumulationCode "nqeAddDlp")]
  =>
  (.info lRuleLogger (str "dlpCounter: " (swap! dlpCounter inc)))
  (insert-unconditional! (Map/of "$volumeAccumulationWriter" ?volumeAccumulation))
  (retract! ?volumeAccumulation)
  )

However, it seems as if the retract isn't actually working because I'll see a burst of 364 log statements with every subsequent call to fireRules from further processing the java code does. I'm only expecting to see 364 log statements adn nothing more since each fact that is handled should be retracted, and should never need to be processed again.
Thoughts?

@victorrodrigueznadq I understand the problem statement here. I think I need to see a bit more about your external process though. Could you show at least an example of the sequence of calls you are making from one fireRules to the next, also including how you are inserting the initial facts into the session.

Sure @mrrodriguez ....creating the session...

<...TRUNCATED...>

@victorrodrigueznadq Thanks for the details. That helps see more of the complete picture.
Where is the fact type VolumeAccumulation coming from? I didn't see that in any of the Java from your example I believe.

In your original problem statement, you mentioned that you are inserting 364 of these VolumeAccumulation facts. That is what I want to understand the source of in the Java above, just so I'm conceptually tracking it correctly.

@victorrodrigueznadq
Copy link
Author

victorrodrigueznadq commented Jun 21, 2024

Still struggling here @EthanEChristian @mrrodriguez ....decided to take a different approach....
Rather than creating the VolumeAccumulation facts from clara, now I'm inserting them (364 of them) from the calling java code, before calling fireRules. And now my rule looks like this:

(defrule add-all-dlp-firm-symbols
  "Add All DLP Firm Symbols"
  {:salience 1000000}
  [?volumeAccumulation <- VolumeAccumulation
   (= accumulationCode "nqeAddDlp")]
  =>
  (.info lRuleLogger (str "dlpCounter: " (swap! dlpCounter inc)))
  (insert-unconditional! (Map/of "$volumeAccumulationWriter" ?volumeAccumulation))
  (retract! ?volumeAccumulation)
  )

However, it seems as if the retract isn't actually working because I'll see a burst of 364 log statements with every subsequent call to fireRules from further processing the java code does. I'm only expecting to see 364 log statements adn nothing more since each fact that is handled should be retracted, and should never need to be processed again.
Thoughts?

@victorrodrigueznadq I understand the problem statement here. I think I need to see a bit more about your external process though. Could you show at least an example of the sequence of calls you are making from one fireRules to the next, also including how you are inserting the initial facts into the session.

Sure @mrrodriguez ....creating the session...
<...TRUNCATED...>

@victorrodrigueznadq Thanks for the details. That helps see more of the complete picture. Where is the fact type VolumeAccumulation coming from? I didn't see that in any of the Java from your example I believe.

In your original problem statement, you mentioned that you are inserting 364 of these VolumeAccumulation facts. That is what I want to understand the source of in the Java above, just so I'm conceptually tracking it correctly.

@mrrodriguez they are inserted during one of the two reference data insert loops.

@victorrodrigueznadq
Copy link
Author

@mrrodriguez we are currently actrively taking measures to work around this, but we need to understand how things like retract and insert work so that we know what's going on. if we try to retract something and it really isn't retracted, we'd like to understand why. things that used to work fine in drools aren't working as expected in clara.

@mrrodriguez
Copy link
Collaborator

@mrrodriguez we are currently actrively taking measures to work around this, but we need to understand how things like retract and insert work so that we know what's going on. if we try to retract something and it really isn't retracted, we'd like to understand why. things that used to work fine in drools aren't working as expected in clara.

Ok, that makes sense. I will try making a small example to demonstrate this type of retraction.


As a bit of an aside:

I also came from a Drools background. There are some key differences in Clara vs Drools that are worth noting. I will try putting of a few of these here (from memory) that hopefully are accurate:

  1. In a rule Right-Hand Side (aka. RHS) the default insert of Clara is a "logical insert" - meaning that the inserted fact is managed by the Truth Maintenance System (aka. the TMS). In Drools, the default insert is an "unconditional insert", ie. where the fact is not managed by the TMS. Both engines allow you to do both operations though.
    In your example, you are of course using r/insert-unconditional! in Clara to have this particular fact not managed by the TMS.

  2. Clara does not attempt to "de-duplicate facts" that are = to be single logical occurrence of the fact. Drools does when. using logical inserts. This mostly would matter if you tried to count these facts later in something like an accumulator or if you use r/retract! with particular expectations. There are pros/cons to these approaches. I discussed this topic more in this past comment Rules [re]run before session updates #469 (comment)

  3. Clara has an immutable working memory and Drools does not.

  4. Clara may delay performing insert/retract operations until r/fire-rules is called on the session. This is done for performance reasons. The implication is that you should never query from a session after inserts/retracts if you haven't first fired the rules to get the session into the expected, well-defined state suitable for the query. Modern Drools does this too I believe, but it didn't in earlier versions.

@victorrodrigueznadq
Copy link
Author

victorrodrigueznadq commented Jun 21, 2024

@mrrodriguez we are currently actrively taking measures to work around this, but we need to understand how things like retract and insert work so that we know what's going on. if we try to retract something and it really isn't retracted, we'd like to understand why. things that used to work fine in drools aren't working as expected in clara.

Ok, that makes sense. I will try making a small example to demonstrate this type of retraction.

As a bit of an aside:

I also came from a Drools background. There are some key differences in Clara vs Drools that are worth noting. I will try putting of a few of these here (from memory) that hopefully are accurate:

  1. In a rule Right-Hand Side (aka. RHS) the default insert of Clara is a "logical insert" - meaning that the inserted fact is managed by the Truth Maintenance System (aka. the TMS). In Drools, the default insert is an "unconditional insert", ie. where the fact is not managed by the TMS. Both engines allow you to do both operations though.
    In your example, you are of course using r/insert-unconditional! in Clara to have this particular fact not managed by the TMS.
  2. Clara does not attempt to "de-duplicate facts" that are = to be single logical occurrence of the fact. Drools does when. using logical inserts. This mostly would matter if you tried to count these facts later in something like an accumulator or if you use r/retract! with particular expectations. There are pros/cons to these approaches. I discussed this topic more in this past comment Rules [re]run before session updates #469 (comment)
  3. Clara has an immutable working memory and Drools does not.
  4. Clara may delay performing insert/retract operations until r/fire-rules is called on the session. This is done for performance reasons. The implication is that you should never query from a session after inserts/retracts if you haven't first fired the rules to get the session into the expected, well-defined state suitable for the query. Modern Drools does this too I believe, but it didn't in earlier versions.

Thanks @mrrodriguez ! What, exactly, does immutable working memory mean? Does it mean you can't and/or shouldn't insert into or retract from it in the RHS? Does it mean that you can't and/or shouldn't call setters on any facts that caused a rule to fire? In Drools, we used flags to prevent any further processing of a fact, but that also isn't working as expected in Clara. Instead of the LHS conditions that check for the flag not being set, we have to also add additional checks in the RHS of our Clara rules to again check that the flag isn't already set.

Example:

LHS:
Only fire for facts that don't have flag X set.

RHS:
Only execute for facts that don't have flag X set {
Do stuff.
Set Flag X.
}

Is there a better way to accomplish the same thing in Clara? (i.e. prevent further other rules from firing for a given fact)

@mrrodriguez
Copy link
Collaborator

@mrrodriguez we are currently actrively taking measures to work around this, but we need to understand how things like retract and insert work so that we know what's going on. if we try to retract something and it really isn't retracted, we'd like to understand why. things that used to work fine in drools aren't working as expected in clara.

Ok, that makes sense. I will try making a small example to demonstrate this type of retraction.

For this part @victorrodrigueznadq I will put an example below of a setup that seems similar to the one you describe (very simplified) to use a r/retract! in the RHS of a rule. This output of the queries after r/fire-rules is what I'd expect.

I put it in this gist for easier reference later: https://gist.github.com/mrrodriguez/87053edaca5ae4b3c547303bb49e77f9

Is there a chance that the object type VolumeAccumulation you are using as a fact perhaps doesn't have a well-formed equals/hashCode implementation?

@victorrodrigueznadq
Copy link
Author

VolumeAccumulation

@mrrodriguez as a matter of fact, it has neither equals nor hashCode. Is it important, in Clara, for facts to have these defined?

@mrrodriguez
Copy link
Collaborator

@mrrodriguez we are currently actrively taking measures to work around this, but we need to understand how things like retract and insert work so that we know what's going on. if we try to retract something and it really isn't retracted, we'd like to understand why. things that used to work fine in drools aren't working as expected in clara.

Ok, that makes sense. I will try making a small example to demonstrate this type of retraction.
As a bit of an aside:
I also came from a Drools background. There are some key differences in Clara vs Drools that are worth noting. I will try putting of a few of these here (from memory) that hopefully are accurate:

  1. In a rule Right-Hand Side (aka. RHS) the default insert of Clara is a "logical insert" - meaning that the inserted fact is managed by the Truth Maintenance System (aka. the TMS). In Drools, the default insert is an "unconditional insert", ie. where the fact is not managed by the TMS. Both engines allow you to do both operations though.
    In your example, you are of course using r/insert-unconditional! in Clara to have this particular fact not managed by the TMS.
  2. Clara does not attempt to "de-duplicate facts" that are = to be single logical occurrence of the fact. Drools does when. using logical inserts. This mostly would matter if you tried to count these facts later in something like an accumulator or if you use r/retract! with particular expectations. There are pros/cons to these approaches. I discussed this topic more in this past comment Rules [re]run before session updates #469 (comment)
  3. Clara has an immutable working memory and Drools does not.
  4. Clara may delay performing insert/retract operations until r/fire-rules is called on the session. This is done for performance reasons. The implication is that you should never query from a session after inserts/retracts if you haven't first fired the rules to get the session into the expected, well-defined state suitable for the query. Modern Drools does this too I believe, but it didn't in earlier versions.

Thanks @mrrodriguez ! What, exactly, does immutable working memory mean? Does it mean you can't and/or shouldn't insert into or retract from it in the RHS? Does it mean that you can't and/or shouldn't call setters on any facts that caused a rule to fire? In Drools, we used flags to prevent any further processing of a fact, but that also isn't working as expected in Clara. Instead of the LHS conditions that check for the flag not being set, we have to also add additional checks in the RHS of our Clara rules to again check that the flag isn't already set.

Example:

LHS: Only fire for facts that don't have flag X set.

RHS: Only execute for facts that don't have flag X set { Do stuff. Set Flag X. }

Is there a better way to accomplish the same thing in Clara? (i.e. prevent further other rules from firing for a given fact)

When I said that working memory is immutable I mean that when you externally insert/retract facts and/or fire rules you get a new working memory state reference back. The original state is not mutated, ie. it is not "updated in place". This is done efficiently internally and the working memory state data structures share a lot of memory (via Clojure's built-in persistent immutable data structures). This means you must always be sure to use the return value when calling these methods too since that is where the change happens.

You bring up something else interesting here though, which is mutating facts that are in working memory. Clara does assume that you are not mutating facts in working memory. They are treated as pure values that do not change. This means you should not call setters on objects that are held in working memory. Changing a fact should be modeled in some other way like mentioned in the approach (1) or (2) in this post https://www.metasimple.org/2017/12/23/clara-updating-facts.html

In Drools you are not supposed to arbitrarily mutate facts either I believe. I think it has special RHS syntax perhaps to let the engine know you are "modifying" a fact. Clara doesn't have this. It is an interesting feature, but not something we've pursued before. It is a simpler and more pure model to have the immutability. There also become a lot of useful properties that come with having immutable facts and an immutable working memory structure.

I think for your case described above you could model it with an intermediate fact that represented the "flagged fact". You'd use more rules to model the situation that causes a fact to become flagged. I don't have a lot of context there to show you more concrete, but hopefully the blog references help.

@mrrodriguez
Copy link
Collaborator

mrrodriguez commented Jun 22, 2024

VolumeAccumulation

@mrrodriguez as a matter of fact, it has neither equals nor hashCode. Is it important, in Clara, for facts to have these defined?

In this particular case, your rule called retract on the same fact binding that you bound in the LHS, so should still work anyways - they are identical object references so should be considered equal. I think in general though that assumption worries me. Clara uses tends to consider facts to have "value semantics". I've mentioned part of this already before, that means they are expected to be immutable when in working memory. The other part of that is they are supposed to have equals/hashCode behavior that reflects your expectation of when you are describing the "same fact". If you do not have a equals/hashCode than you are only able to use object identity to determine if a fact is the same in working memory. Perhaps that is what you want, but I think often that is not the case.

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

No branches or pull requests

2 participants