You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up to #6216 (PR #6222). That issue named three procedures - algo.node2vec, algo.maxKCut, algo.influenceMaximization - and they are fixed. The same two defects are present, unchanged, in thirteen more procedures that were never in scope.
The two defects
#6216 established that an iteration-shaped knob needs two things:
A domain minimum, rejected by name. Below its minimum such a knob does not mean "a smaller run", it means an answer the algorithm cannot produce - and the value is absorbed in silence. algo.maxKCut(2, {restarts: 0}) used to return every node in community 0 with a cut weight of -1.0, reported as success.
A checkpoint inside the loop it drives, so a large value is abortable via thread interrupt and arcadedb.command.timeout rather than forbidden by a guessed cap.
The thirteen
Every one of these extracts its knob with a plain extractInt(n, "maxIterations") - no minimum - and contains zero references to WorkGuard:
procedure
knob
default
AlgoPageRank
maxIterations
20
AlgoPersonalizedPageRank
maxIterations
20
AlgoArticleRank
maxIterations
20
AlgoEigenvectorCentrality
maxIterations
100
AlgoHITS
maxIterations
20
AlgoKatz
maxIterations
10
AlgoLouvain
maxIterations
10
AlgoLeiden
maxIterations
10
AlgoLabelPropagation
maxIterations
10
AlgoSLPA
iterations
20
AlgoSimRank
maxIterations
5
AlgoFastRP
iterations
4
AlgoHashGNN
iterations
4
Verified against the merged tree: grep -ln "maxIterations|\"iterations\"" *.java returns 15 files, and only AlgoNode2Vec and AlgoMaxKCut - the two #6216 touched - contain any guard. call.
So CALL algo.pageRank({maxIterations: 0}) returns the uniform initial rank vector as though it were a PageRank result, and CALL algo.pageRank({maxIterations: 2000000000}) spins with no way to interrupt it or to have arcadedb.command.timeout observed. Both are what #6216 fixed for algo.maxKCut.
The silent-wrong-answer half is the more serious one: an unconverged or un-iterated centrality is not obviously wrong to a caller, unlike an exception.
Why it was out of scope then and should be in scope now
#6216 was scoped to the knobs its parent review had actually named, and #6222 deliberately did not widen it - a validation fix that also rewrites thirteen unrelated procedures is hard to review, and the mechanism it needed (extractInt(value, name, minimum), WorkGuard, checkPeriodically) did not exist yet. It exists now and is in AbstractAlgoProcedure, so this is mechanical application rather than design.
Notes for whoever picks this up
Three things #6222 learned the hard way, all of which apply here:
A checkpoint goes inside the loop whose trip count the knob controls, not around it.fix(#6216): a graph-algorithm knob is bounded by the resource it spends, not by a guessed cap #6222 spent three review cycles on this: a checkpoint between walks does not bound a single walk that is O(walkLength squared). For an iterative centrality the outer iteration loop is usually the right place and the per-node scan within an iteration, since the latter is O(nodeCount).
Throttle with checkPeriodically only where one iteration can be cheaper than the check itself. Where an iteration already allocates or scans the graph, the unthrottled check() gives better abort latency at no measurable cost.
Some of these procedures also have a tolerance convergence check that may exit before maxIterations, so a test driving the timeout needs a tolerance low enough that it does not converge early.
Follow-up to #6216 (PR #6222). That issue named three procedures -
algo.node2vec,algo.maxKCut,algo.influenceMaximization- and they are fixed. The same two defects are present, unchanged, in thirteen more procedures that were never in scope.The two defects
#6216 established that an iteration-shaped knob needs two things:
algo.maxKCut(2, {restarts: 0})used to return every node in community 0 with a cut weight of-1.0, reported as success.arcadedb.command.timeoutrather than forbidden by a guessed cap.The thirteen
Every one of these extracts its knob with a plain
extractInt(n, "maxIterations")- no minimum - and contains zero references toWorkGuard:AlgoPageRankmaxIterationsAlgoPersonalizedPageRankmaxIterationsAlgoArticleRankmaxIterationsAlgoEigenvectorCentralitymaxIterationsAlgoHITSmaxIterationsAlgoKatzmaxIterationsAlgoLouvainmaxIterationsAlgoLeidenmaxIterationsAlgoLabelPropagationmaxIterationsAlgoSLPAiterationsAlgoSimRankmaxIterationsAlgoFastRPiterationsAlgoHashGNNiterationsVerified against the merged tree:
grep -ln "maxIterations|\"iterations\"" *.javareturns 15 files, and onlyAlgoNode2VecandAlgoMaxKCut- the two #6216 touched - contain anyguard.call.So
CALL algo.pageRank({maxIterations: 0})returns the uniform initial rank vector as though it were a PageRank result, andCALL algo.pageRank({maxIterations: 2000000000})spins with no way to interrupt it or to havearcadedb.command.timeoutobserved. Both are what #6216 fixed foralgo.maxKCut.The silent-wrong-answer half is the more serious one: an unconverged or un-iterated centrality is not obviously wrong to a caller, unlike an exception.
Why it was out of scope then and should be in scope now
#6216 was scoped to the knobs its parent review had actually named, and #6222 deliberately did not widen it - a validation fix that also rewrites thirteen unrelated procedures is hard to review, and the mechanism it needed (
extractInt(value, name, minimum),WorkGuard,checkPeriodically) did not exist yet. It exists now and is inAbstractAlgoProcedure, so this is mechanical application rather than design.Notes for whoever picks this up
Three things #6222 learned the hard way, all of which apply here:
checkPeriodicallyonly where one iteration can be cheaper than the check itself. Where an iteration already allocates or scans the graph, the unthrottledcheck()gives better abort latency at no measurable cost.Some of these procedures also have a
toleranceconvergence check that may exit beforemaxIterations, so a test driving the timeout needs a tolerance low enough that it does not converge early.Related
#5924, #6065, #6216, PR #6055, PR #6214, PR #6222.