eclipse-viatra / org.eclipse.viatra

Main components of the VIATRA framework
https://eclipse.dev/viatra
Eclipse Public License 2.0
0 stars 1 forks source link

Unexpected Behaviour first time calling DelayUpdatePropagation() #123

Open eclipse-viatra-bot opened 4 months ago

eclipse-viatra-bot commented 4 months ago

| --- | --- | | Bugzilla Link | 563985 | | Status | NEW | | Importance | P3 normal | | Reported | Jun 05, 2020 08:59 EDT | | Modified | Jul 14, 2020 04:37 EDT | | Version | 2.3.2 | | Reporter | Hans van der Laan |

Description

Hey,

I’m experimenting with the “delayUpdatePropagation()” method, and I’m getting some behavior I’m not sure how to explain.

The first time I'm passing a callable to delayUpdatePropagation(), every time I call fireOne(rule) (with any rule) in call() nothing happens. All subsequent times I’m passing a callable to delayUpdatePropagation(), every time I call fireOne(rule) in call() the rule is executed as expected.

I’ve uploaded a minimal example program which showcases this behavior: https://github.com/HansvdLaan/viatra-bug/tree/master/com.vanderhighway.trbac.verifier/src/main/java/com/vanderhighway/trbac/verifier

For every “DayRange” match, this program creates a “Role” with an arbitrary name a “ScheduleRange” which is a copy of the “DayRange” entity.

Output:\ [ADD Range Match] "range"=Monday-[31,60], "starttime"=31, "endtime"=60 \ […]\ [ADD Range Match] "range"=2J[41-60], "starttime"=41, "endtime"=60 \ EventDrivenTransformationRule called, with:Match{"range"=Monday-[31,60], "starttime"=31, "endtime"=60}\ EventDrivenTransformationRule called, with:Match{"range"=2J[41-60], "starttime"=41, "endtime"=60}\ [ADD RoleName Match] "role"=Role-2J[41-60], "name"=Role-2J[41-60] \ [ADD ScheduleRange Match] "range"=copy-com.vanderhighway.trbac.model.trbac.model.impl.RangeImpl@593aaf41 (starttime: 41, endtime: 60, name: 2J[41-60]), "starttime"=41, "endtime"=60 \ EventDrivenTransformationRule called, with:Match{"range"=1J[0-40], "starttime"=0, "endtime"=40}\ [ADD RoleName Match] "role"=Role-1J[0-40], "name"=Role-1J[0-40] \ [ADD ScheduleRange Match] "range"=copy-com.vanderhighway.trbac.model.trbac.model.impl.RangeImpl@485966cc (starttime: 0, endtime: 40, name: 1J[0-40]), "starttime"=0, "endtime"=40 \ […]\ EventDrivenTransformationRule called, with:Match{"range"=1J[41-60], "starttime"=41, "endtime"=60}\ [ADD RoleName Match] "role"=Role-1J[41-60], "name"=Role-1J[41-60] \ [ADD ScheduleRange Match] "range"=copy-com.vanderhighway.trbac.model.trbac.model.impl.RangeImpl@1de76cc7 (starttime: 41, endtime: 60, name: 1J[41-60]), "starttime"=41, "endtime"=60 \ Done!

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 12, 2020 08:13

Could anyone confirm if this is a bug, or if it's due to the fact that I'm using the API in an unintended way?

There is also more to this problem then I initially thought. It seems as also sometimes instead of first, the last callable isn't run.

eclipse-viatra-bot commented 4 months ago

By Zoltan Ujhelyi on Jun 12, 2020 09:47

Sorry for missing the original report, I was a bit swamped last week.

About your issue, it is not really clear to me what you would like to achieve, as you are combining elements of our API that are not really meant to be used together:

Could you please clarify first what was it you were planning to achieve with the delayUpdatePropagation() call?

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 12, 2020 10:41

Hey Zoltan,

I’m trying to achieve the following:

My model contains a set of time intervals, e.g.:\ T = {interval1=[0,10], interval2=[10,20], interval3=[20,30]}

We want to know the set of “scenario’s”, i.e. which intervals overlap and when they overlap. For S, this would be:\ S = {[0,9] -> {interval1}, [10,10] -> {interval1,interval2}, [10,19] -> {interval2}, [20,20] -> {interval2, interval3}, [21,30] -> {interval3}}

When a user adds/removes/resizes an interval in T, S is updated. We have an incremental algorithm to update S based on the changes to T. However, one modification to T may result in multiple changes to S. We do not want to re-evaluate any queries before all the changes to S have been applied. Otherwise, we will do many unnecessary calculations.

Now, regarding the implementation:

In 10.2 of the query-api documentation, it is stated that: “As of version 1.6, the advanced query API now includes a feature that lets users temporarily "turn off" query result maintenance in the incremental query backend. During such a code block, only the base model indexer is updated, query results remain stale until the end of the block. The advantage is that it is possible to save significant execution time when changing the model in a way that partially undoes itself, e.g. a large part of the model is removed and then re-added.”. This made it seem as a way to turn-off query re-evaluation until all changes have been applied.

I’m using EventDrivenTransformations for transformations which are triggered based on graph-pattern matches being added/removed. I’m using BatchTransformationRules for all the (sub)transformations I want to fire manually.

For example: when a new interval is added into T, this is detected via a graph pattern and a EventDrivenTransformationRule is triggered. The transformations on S inside the EventDrivenTransformationRule are BatchTransformationRules. I also use BatchTransformationRules to deal with user input, e.g. to add/remove model entities and model relations.

eclipse-viatra-bot commented 4 months ago

By Gabor Bergmann on Jun 12, 2020 10:49

  • The AdvancedQueryEngine#delayUpdatePropagation is a very low-level method that was introduced to extend the support for recursive queries in VIATRA, and as far as I can tell, there should be no real reason for it to be called by end-users (but I should check with Gábor Bergmann about this, here I might be wrong).

Well, we intended it to be available to end users. It is a way to improve performance by coalescing a bunch of updates, some of which may "cancel" each other in their effects. You can save the incremental query engine some work by telling it to process all these updates in one go. This can help to avoid costly transients, e.g. a lot of matches appearing and then disappearing. Is such a performance problem your motivation for using it?

As said in the Javadoc for AdvancedQueryEngine#delayUpdatePropagation, "Within the callback, these backends will provide stale results.". It is entirely possible that there are no matches for the precondition of the batch xform rules at the beginning of the first invocation of delayUpdatePropagation. \ Are you sure the model should support a precondition match at that point?

If yes, one more thing. If the relevant part of the Rete network has not been constructed before delayUpdatePropagation (or even worse, the required types from the metamodel were not yet indexed at that point), then the backend will have no matches to return; it will be "stale" as in empty. (Maybe the Javadoc should clarify this.) If this is the case, an engine.prepareGroup() call during initialization time could help.

I second Zoltán: it is very difficult to discern your motivations behind calling batch transformation rules within an event-driven reaction. Not forbidden, just weird. And why do you construct single-use batch xform rules in the middle of an event-driven rule? You do realize you can just manipulate the model in the action lambda for the rule? You do realize you can just pass a lambda to delayUpdatePropagation as well? Or is this weirdness just there to demonstrate the problem for us?

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 16, 2020 08:57

Are you sure the model should support a precondition match at that point?

Yes, the preconditions should hold. I’m matching on the general policy element, i.e. the element which contains all other elements. Seeing your reaction, this is probably an anti-pattern. When I started with VIATRA, I didn’t know I could also get all elements via Resource.getContents().

If the relevant part of the Rete network has not been constructed before delayUpdatePropagation (or even worse, the required types from the metamodel were not yet indexed at that point), then the backend will have no matches to return; it will be "stale" as in empty. (Maybe the Javadoc should clarify this.) If this is the case, an engine.prepareGroup() call during initialization time could help.

I don’t know causes “parts of the network to be constructed”. Thus, I’m not sure if this is the case. At the moment, I’m not adding/removing constraints at runtime, so I expect it to be fully constructed.

I second Zoltán: it is very difficult to discern your motivations behind calling batch transformation rules within an event-driven reaction. Not forbidden, just weird.

Again, my implementation just grew this way. I defined my model transformations as batchtransformations. As I wanted to reuse them in the event transformations, I just called them again.

And why do you construct single-use batch xform rules in the middle of an event-driven rule? You do realize you can just manipulate the model in the action lambda for the rule? You do realize you can just pass a lambda to delayUpdatePropagation as well? Or is this weirdness just there to demonstrate the problem for us?

No, I did not. Thanks for the heads-up! 😊

eclipse-viatra-bot commented 4 months ago

By Gabor Bergmann on Jun 16, 2020 09:53

I don’t know causes “parts of the network to be constructed”.

  1. Query evaluation may grow both the Rete network and the underlying EMF base index on demand, when you ask for matches of patterns that the engine has not seen before.

When inside “delayUpdatePropagation()”, no update propagation happens, so this dynamic growing will not properly finish until you leave “delayUpdatePropagation()”. In the mean time, results will be the same as if you requested them before “delayUpdatePropagation()” - for queries that the engine has never seen, this stale result will be empty.

  1. You can also explicitly call engine.prepareGroup(PolicyQueries.INSTANCE) when you are setting up the engine / transformation, before any rule activation. This causes the Rete network for these patterns to be already constructed when you call “delayUpdatePropagation()”, so that the returned results will correctly reflect the state the model had at the beginning of “delayUpdatePropagation()”.

This is what I was trying to explain in the previous comment.

At the moment, I’m not adding/removing constraints at runtime

It is possible that you do, when you use a patter for the first time.

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 17, 2020 06:15

I've rewritten my implementation:

Quick question: If you manipulate the model inside the delayUpdatePropagation() method, is it expected behavior for the EventDrivenTransformations to only be triggered when the next manipulation outside of the previous delayUpdatePropagation() happens instead of being triggered after the callable has finished?

E.g. When imagine we have the following set of manipulations:\ Range testRange1 = engine.delayUpdatePropagation( () -> { return modifier.addRange(group, monday, "testRange1", new IntegerInterval(10,20)); } );\ Range testRange2 = engine.delayUpdatePropagation( () -> { return modifier.addRange(group, monday, "testRange2", new IntegerInterval(20,30)); } );

Then, we will have the following sequence of events:\ Range testRange1 is created.\ Range testRange2 is created.\ EventDrivenTransformation is triggered with match testRange1

Also, it seems as the matches which triggers the EventDrivenTransformation is not send to the MatchUpdateListener. Not even the matches which have triggered an EventDrivenTransformation.

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 22, 2020 05:12

If somebody can confirm this is not supposed to happen, I can create a more minimalistic example which showcases this behavior.

If this is supposed to happen: What is the best way to trigger the update propagation after you exit the delayUpdatePropagation()?

eclipse-viatra-bot commented 4 months ago

By Zoltan Ujhelyi on Jun 22, 2020 07:04

(In reply to Hans van der Laan from comment #8)

If somebody can confirm this is not supposed to happen, I can create a more minimalistic example which showcases this behavior.

If this is supposed to happen: What is the best way to trigger the update propagation after you exit the delayUpdatePropagation()?

The notifications should be propagated after delayUpdatePropagation() and I am reasonably sure this is what happens in the query engine. However, there might be some unexpected surprises with the transformation engine, so a minimal example would be very helpful.

I was planning to allocate some time to debug this issue, but it won't happen until July.

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 22, 2020 08:34

Okay, I will make another smaller example later this week.

eclipse-viatra-bot commented 4 months ago

By Gabor Bergmann on Jun 22, 2020 11:12

At the end of delayUpdatePropagation(), the query engine should update its stored match sets, and report the correct results. (Can you confirm this is true?)

If the precondition pattern of a rule has a new match, then that rule gains a new ACTIVATION.

However, this is notable different from the rule actually being executed. No rules are fired until the event-driven VM is scheduled.

Such a scheduling could correspond to, say, the end of an EMF transaction, or a manual call. The most commonly used option is to schedule it right after any change notification from the EMF model. But clearly this is not the right choice for you, if at that point you still wish to preserve the old contents of the match sets: delayUpdatePropagation() causes the event-driven VM to see a stale picture of the match sets of the precondition patterns, at the point in time when it is scheduled.

If you insist on using delayUpdatePropagation(), you cannot rely on any mechanism that would trigger rule executions in the middle of such a block, so in this case you need to find another scheduler that suits your use case.

See a high-level overview of the concepts such as "scheduling" here: https://www.eclipse.org/viatra/documentation/evm.html

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 24, 2020 09:05

At the end of delayUpdatePropagation(), the query engine should update its stored match sets, and report the correct results. (Can you confirm this is true?)

Yes, the match sets are correctly updated.

The most commonly used option is to schedule it right after any change notification from the EMF model. But clearly this is not the right choice for you, if at that point you still wish to preserve the old contents of the match sets

If you insist on using delayUpdatePropagation(), you cannot rely on any mechanism that would trigger rule executions in the middle of such a block, so in this case you need to find another scheduler that suits your use case.

I’m not sure I understand what you mean, but as far as I see I'm not trying to preserve old contents of the match set nor trigger rule executions inside of the block. Instead, I just want the transformations to be triggered after I’ve left the delayUpdatePropagation().

The use case is that when I’m adding/updating certain model entities I have to set multiple attributes/relations at once. The EventDrivenTransformations should not be triggered while I'm still "working on" the model entities.

eclipse-viatra-bot commented 4 months ago

By Zoltan Ujhelyi on Jun 24, 2020 11:45

(In reply to Hans van der Laan from comment #12)

At the end of delayUpdatePropagation(), the query engine should update its stored match sets, and report the correct results. (Can you confirm this is true?)

Yes, the match sets are correctly updated.

This is good news, as then we only have to consider when the EVM rules are getting fired.

The most commonly used option is to schedule it right after any change notification from the EMF model. But clearly this is not the right choice for you, if at that point you still wish to preserve the old contents of the match sets

If you insist on using delayUpdatePropagation(), you cannot rely on any mechanism that would trigger rule executions in the middle of such a block, so in this case you need to find another scheduler that suits your use case.

I’m not sure I understand what you mean, but as far as I see I'm not trying to preserve old contents of the match set nor trigger rule executions inside of the block. Instead, I just want the transformations to be triggered after I’ve left the delayUpdatePropagation().

The use case is that when I’m adding/updating certain model entities I have to set multiple attributes/relations at once. The EventDrivenTransformations should not be triggered while I'm still "working on" the model entities.

So basically you have something like a transaction and you only want to fire the event-driven transformation rules on the end of these transactions. The event-driven transformations in VIATRA work with two kinds of events:

The issue you have seen is either a bug that delayUpdatePropagation does not trigger the default scheduler as expected or there may be a valid reason for the behaviour that is hard to see in your code - this is the part I wanted to test in more detail (and here is where a simpler example would be very helpful).

In the meantime, you could achieve this kind delayed evaluation by implementing a custom scheduler of your event-driven transformation that can be triggered by you.

eclipse-viatra-bot commented 4 months ago

By Gabor Bergmann on Jun 24, 2020 15:54

delayUpdatePropagation does not trigger the default scheduler as expected

I do not think it is expected to. It is expected to update match sets, which does happen. But the default scheduler is bound to EMF notifications, not to the query engine.

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 26, 2020 06:58

I’ve uploaded a new (and smaller) example to github:

https://github.com/HansvdLaan/viatra-bug-2

What this example does is the following:

We have day schedules, and day schedules have time ranges associated with them. These are essentially named time intervals. E.g. time_range_1 = [0,10], time_range_2 = [20,30], etc. For each time range, we want to create a “mirror”. Namely, we want to create an “DayScheduleTimeRange” containing the same time range. When we update a time range, its mirror should also be updated. When we remove a time range, its mirror should be removed.

The code which adds the time ranges (in PolicyValidatorMain):

DaySchedule monday = (DaySchedule) resource.getEObject("Monday");\ TimeRangeGroup group = modifier.addTimeRangeGroup("DummyTimeRangeGroup");

engine.delayUpdatePropagation(() -> modifier.addTimeRange(group, monday, "time_range_1", new IntegerInterval(0,10)));\ engine.delayUpdatePropagation(() -> modifier.addTimeRange(group, monday, "time_range_2", new IntegerInterval(20,30)));\ engine.delayUpdatePropagation(() -> modifier.addTimeRange(group, monday, "time_range_3", new IntegerInterval(40,50)));

TimeRange timeRange3 = (TimeRange) resource.getEObject("time_range_3");\ engine.delayUpdatePropagation(() -> modifier.updateTimeRange(timeRange3, new IntegerInterval(42,42)));\ engine.delayUpdatePropagation(() -> { modifier.removeTimeRange(timeRange3); return null; });

The code which processes the changes (in PolicyAutomaticModifier):

private EventDrivenTransformationRule<TimeRangeP.Match, TimeRangeP.Matcher> ProcessRanges() {\ EventDrivenTransformationRule<TimeRangeP.Match, TimeRangeP.Matcher> dayrangerule =\ this._eventDrivenTransformationRuleFactory.createRule(TimeRangeP.instance()).action(\ CRUDActivationStateEnum.CREATED, (TimeRangeP.Match it) -> {\ try {\ System.out.println("TRIGGERED EventDrivenTransformationRule TimeRangeP CREATED:" + it.prettyPrint());\ this.policyModifier.addDayScheduleTimeRange(it.getDaySchedule(),\ it.getTimeRange().getName(), new IntegerInterval(it.getStarttime(), it.getEndtime()));\ } catch (ModelManipulationException e) {\ e.printStackTrace();\ }\ }).action(\ CRUDActivationStateEnum.UPDATED, (TimeRangeP.Match it) -> {\ try {\ System.out.println("TRIGGERED EventDrivenTransformationRule TimeRangeP UPDATED:" + it.prettyPrint());\ this.policyModifier.updateDayScheduleTimeRange(it.getTimeRange().getDayScheduleTimeRanges().get(0), //In this example, it's always just one!\ new IntegerInterval(it.getStarttime(), it.getEndtime()));\ } catch (ModelManipulationException e) {\ e.printStackTrace();\ }\ }).action(\ CRUDActivationStateEnum.DELETED, (TimeRangeP.Match it) -> {\ System.out.println("TRIGGERED EventDrivenTransformationRule TimeRangeP DELETED:" + it.prettyPrint());\ try {\ DayScheduleTimeRange dayScheduleTimeRange = it.getTimeRange().getDayScheduleTimeRanges().get(0); //In this example, it's always just one!\ this.policyModifier.removeDayScheduleTimeRange(dayScheduleTimeRange);\ } catch (ModelManipulationException e) {\ e.printStackTrace();\ }\ } ).addLifeCycle(Lifecycles.getDefault(true, true)) .name("process-timeranges").build();\ return dayrangerule;\ }

The event driven transformation rule should only be called AFTER the time range add/update/remove method has been fully executed, i.e. the “transaction” has been completed.

At the moment, this is the current output:

[ADD TimeRangeP Match] "daySchedule"=Monday, "group"=DummyTimeRangeGroup, "timeRange"=time_range_1, "starttime"=0, "endtime"=10 \ TRIGGERED EventDrivenTransformationRule TimeRangeP CREATED:"daySchedule"=Monday, "group"=DummyTimeRangeGroup, "timeRange"=time_range_1, "starttime"=0, "endtime"=10\ TRIGGERED EventDrivenTransformationRule TimeRangeP UPDATED:"daySchedule"=Monday, "group"=DummyTimeRangeGroup, "timeRange"=time_range_1, "starttime"=0, "endtime"=10\ [ADD TimeRangeP Match] "daySchedule"=Monday, "group"=DummyTimeRangeGroup, "timeRange"=time_range_2, "starttime"=20, "endtime"=30 \ [ADD DayScheduleTimeRangeP Match] "daySchedule"=Monday, "dstr"=time_range_1, "starttime"=0, "endtime"=10 \ [ADD TimeRangeP Match] "daySchedule"=Monday, "group"=DummyTimeRangeGroup, "timeRange"=time_range_3, "starttime"=30, "endtime"=40 \ [REM TimeRangeP Match] "daySchedule"=Monday, "group"=DummyTimeRangeGroup, "timeRange"=time_range_3, "starttime"=30, "endtime"=40 \ [ADD TimeRangeP Match] "daySchedule"=Monday, "group"=DummyTimeRangeGroup, "timeRange"=time_range_3, "starttime"=42, "endtime"=42 \ [REM TimeRangeP Match] "daySchedule"=Monday, "group"=DummyTimeRangeGroup, "timeRange"=time_range_3, "starttime"=42, "endtime"=42 \ Done!

Previously, I wrapped the IModelManipulations inside of batchtransformations but I got the impression that this isn’t necessary. \ Now interestingly for this example, if I don’t wrap them inside engine.delayUpdatePropagation I get the same output.

we have also implemented a scheduler that reacts to EMF Transaction commits

On a small side-note, I get the impression that this is the kind of scheduler I need. Do you have an example where you use this scheduler and show how you can create/invoke EMF transactions?

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jun 26, 2020 07:23

On a small side-note, for this small example it isn't the case that only the last event driven transformation doesn't get executed.

In the application I'm working on, this is the case. The only difference between my application and the minimal example is that instead of making a simple mirror I do a more complex operation which can generate/remove/update multiple “DayScheduleTimeRange”. If you want, I can also upload this application. However, it's far from a minimal exmaple.

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jul 02, 2020 07:32

On a small side-note, I get the impression that this is the kind of scheduler I need. Do you have an example where you use this scheduler and show how you can create/invoke EMF transactions?

Shall I re-ask this question in the forums? I can imagine that using transactions is quite a common use-case scenario. Probably, more people would benefit from having this knowledge. This way, it's easily findable.

eclipse-viatra-bot commented 4 months ago

By Gabor Bergmann on Jul 02, 2020 15:56

I do not think it is expected to. It is expected to update match sets, which does happen. But the default scheduler is bound to EMF notifications, not to the query engine.

I was wrong, the default should update after the query engine. Then this is a bug indeed. We should fix delayUpdatePropagation so that the listener / EVM is triggered properly at the end.

On a small side-note, I get the impression that this is the kind of scheduler I need. Do you have an example where you use this scheduler and show how you can create/invoke EMF transactions?

If you do not integrate with a (typically graphical) model editor that already hosts a TransactionalEditingDomain (e.g. Sirius editors are like that), then count your blessings :) TransactionalEditingDomain is difficult to work with.

Until we fix the bug above, you could just manually trigger unscheduled executions of the EVM as a workaround (at the exact point where your "transaction" ends, i.e. when you exit the delay block). Like this:

queryEngine.delayUpdatePropagation( () -> ... );\ eventDrivenTranformation.getExecutionSchema().startUnscheduledExecution();

What the second line does: it looks if there are any event-driven rules with activations (precondition matches for which the rule has not fired yet), and fires them. So basically the same thing that normally happens after a model update.

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jul 03, 2020 03:30

queryEngine.delayUpdatePropagation( () -> ... );

eventDrivenTranformation.getExecutionSchema().startUnscheduledExecution();

Thanks for the workaround!

eclipse-viatra-bot commented 4 months ago

By Hans van der Laan on Jul 14, 2020 04:25

Hey,

This Friday I'm going on holiday for a few weeks. Is there anything you need/would like from me regarding this bug? As I'm not taking my working laptop with me, I won't be able to do much when I'm on holiday.

eclipse-viatra-bot commented 4 months ago

By Zoltan Ujhelyi on Jul 14, 2020 04:37

(In reply to Hans van der Laan from comment #20)

Hey,

This Friday I'm going on holiday for a few weeks. Is there anything you need/would like from me regarding this bug? As I'm not taking my working laptop with me, I won't be able to do much when I'm on holiday.

  • Hans

Hi,

I think we are fine, we understand the basic issue and what we need to do is reproduce it and either state why this is the extended behavior or turn it to a VIATRA test case with an appropriate bugfix. I guess we can do both (given we manage to allocate enough time for it).

Thanks for pointing out the case and your helpful comment. Have a very nice holiday!