Method order does not change when filter ranks change, leading to subtle conflict between nq and lpres

[fingolfin]

We recently observed a test failure with the nq tests after LoadAllPackages() had run. Here is the cause:

First off, the lpres package installs another method for NilpotentQuotient but at a slightly lower priority (adjusting the rank by -1):

InstallOtherMethod( NilpotentQuotient,
  "for an FpGroup using the LPRES-package", true,
  [ IsFpGroup, IsPosInt ], -1, # give priority to NQ package
  function( G, c )
  return( NilpotentQuotient( Range( IsomorphismLpGroup( G ) ), c ) );
  end);

If you just load nq and lpres, there is no problem.

But if you first load nq; then load e.g. semigroups, then load lpres, suddenly the lpres method for NilpotentQuotient will be preferred over nq's -- because semigroups installs implications for IsFpGroup, which increases the rank of IsFpGroup, which increases the rank of the above NilpotentQuotient at the moment it is installed...

So a quick workaround would be to change the -1 above to a lower value. In plain GAP, the rank of IsFpGroup right now seems to be 38. After LoadAllPackages it has rank 111; so we'd need at least something like -80 or so to make it "safe", but of course if enough implications for IsFpGroup is installed, any fixed rank adjustment could become too small.

IMHO in this particular case, the proper fix is to just delete that method from lpres - I see no point in it anyway, as no normal user would ever be able to call it that way (at least once the issue reported here has been resolved). Somebody who wants to compare the lpres nq implementation with that in the nq package could / should just use NilpotentQuotient( Range( IsomorphismLpGroup( G ) ), c ). @laurentbartholdi any objections to that?

But this still leaves the underlying issue here, which is is that the rank of filters can change, but we never adjust the rank of methods based on that, once they have been installed, nor their order.

One fix would be to reorder all methods of all operations whenever a new implication is installed, or some variation thereof. Of course this may expose more issues in method installation which used a hard coded rank adjustment. We'd have to adjust them one by one.

[stevelinton]

Running over all the methods installed (after starting master with standard packages) there are something like 240 places where the relative rank of methods changes when you call WITH_HIDDEN_IMPS_FLAGS on all the requirements. In most cases it looks like the methods which switch apply to disjoint sets of objects, for instance permutation group methods now rank slightly higher than matrix group methods.

[fingolfin]

Here is another complication: we cannot just iterate over all method and adjust their ranks, because we need to know the original rank adjustment passed to InstallMethod; of course we can record that, but that's not quite enough either, because that value may have been computed, e.g. it may have been -RankFilter(IsFinite).

So, in order to be able to adjust the ranks later on, we'd need a completely new way to specify rank adjustments in such a way that we can recompute the adjustment later on.

[stevelinton]

Ouch. We can record the adjustments, but I think unless we want rewrite every method installation in every package, we will just have to accept that the RankFilter in InstallMethod( <op>, <reqs>, RankFilter(<filt>)) is evaluated at the time the InstallMethod is called. This might, possibly, create some dependencies on package loading order, but it should be a lot rarer than at present.

[stevelinton]

An idea which came up in discussion is to allow a function of no arguments in place of the rank adjustment. This is evaluated to produce the actual rank adjustment, but can be recomputed when method order is being recalculated.

[stevelinton]

I've been playing with this in #2521 and I'm no longer sure we even want to fix this issue. If we ensure that method ranking always reflects current implications (and so current filter ranks) then we reach a position where latent bugs (in some sense) in the library can be revealed by a user installing implications (perfectly correctly) which change ranks enough to reorder library methods.

If we do nothing then this issue persists and the relative ranking of a method installed in a package compared to methods in the library or in other packages, may depend on the order in which packages are loaded.

Neither of these is desirable, but I am wondering if the second one isn't the lesser of the two evils. At least the problem arises in relation to a method you have "just" installed, rather than code deep inside the library.