[🐛 Bug Report]: Cycle breaking of recursive rules

[erhc]

Describe the bug

I am trying to implement a recursive path rule on the Mutagenesis dataset, and the rules are defined as below:

template.add_rules([
    (R.bond_embed(V.B)[3,] <= R.get(bond)(V.B)) for bond in ["b_1", "b_2", "b_3", "b_4", "b_5", "b_7"]
])

template += R.path(V.X, V.Y) <= (R.bond(V.X, V.Y, V.B), R.bond_embed(V.B)[3, 3]) 
template += R.path(V.X, V.Y) <= (R.bond(V.X, V.Z, V.B), R.bond_embed(V.B)[3, 3], R.path(V.Z, V.Y)[3, 3])

The problem is that when I have weights in the second rule, it cannot build the dataset and I get the following error:

Without weights, it does not produce any errors.

Steps to reproduce the behavior

Make a template with these rules and try to build the dataset.

Expected behavior

The dataset can be built with the given weighted rules.

Environment

No response

Additional context

No response

[LukasZahradnik]

Thanks for the issue. I was able to reproduce it.

The issue is caused by pruning cycles of weighted neurons. I opened a PR with a fix in the backend repo: https://github.com/GustikS/NeuraLogic/pull/37

[GustikS]

Hi @erhc, thanks a lot for this bug report, this was indeed a real bug (resolved now).

We used to encounter the order sensitivity before due to Value broadcasting, which is what I suspected at first when we talked, but that was indeed already resolved some time ago (as I remembered). This is different - due to breaking cycles in the ground neural network, which is something you definitely do not want...

This points out that we should propagate more info from the backend - it throws a lot of warnings that there are cycles in the ground network caused by this template, but at the frontend it's just a generic java error... should @erhc know this template is causing (infinite) cycles, it would give a valuable feedback that this is not the way to go, which is also closely related to the open discussion https://github.com/LukasZahradnik/PyNeuraLogic/discussions/45

[erhc]

I understand, the rule is not taking only the shortest path. Is there a way to stop the rule from firing after the first found path? Do you have any other implementation recommendation for this? I would definitely avoid having to define a rule for a path of each length separately. Thank you!

[LukasZahradnik]

@GustikS PyNeuraLogic, by default, propagates error logs from NeuraLogic. The cycle breaking is logged as a warning, so it's not visible by default without setting PyNeuraLogic to display warnings.

[GustikS]

@erhc there is no inherent ordering in this problem (or graphs in general), hence to find the shortest path you have to ground all the paths, unfortunatelly, which is the default behavior of the grounder (you can then propagate through the shortest one based on the values, e.g. as shown in documentation).

Depends on what you're trying to do. Here you're basically defining a recursive network on a graph with cycles (these are normally limited to sequences or trees), hence you end up with an infinite recursion = loop in the neural network - the grounder detects that, and removes the last edge that closes the cycle to make it valid - this is kind of arbitrary, hence the warning that this is probably not what you want (but might be in some cases). So to wrap up, you have to think first how exactly you want to define recursive paths in a graph with loops without them being infinite - if you search for a path starting in a specific node (e.g. some query) then the terminal condition can check against it, this is generalized by removing the last edge (any that closes a loop), in which case you can continue as is. An alternative is treating cycles separately by defining the loops of certain length to begin with.

@LukasZahradnik Ok, maybe we could downshift to warnings, too? Or better yet make it somehow more obvious that once they encounter weird problems, switching on backend logs would be a good idea...

[LukasZahradnik]

@GustikS Setting the default level to warnings would cause too much spam, imo (for example, breaking cycles can spam a lot). We would have to review the whole logging and decide proper levels.

Or better yet make it somehow more obvious that once they encounter weird problems, switching on backend logs would be a good idea...

We can catch all Java Exceptions and add a message to them telling users to enable a lower level of logging for more info.

[GustikS]

We can catch all Java Exceptions and add a message to them telling users to enable a lower level of logging for more info.

oh nice, that's a really good solution, I like that!

[LukasZahradnik]

@erhc Hello, there is a new version (0.7.4) that should contain a fix for this issue. Can you please verify that it works? Thanks!

[erhc]

Hello, thank you for your feedback, it is working now!