Add new StatusCode category to return filterpassed states from functional algorithms

Add a new FunctionalFilterDecision StatusCode category to return an algorithm's filterPassed state.
This allows LHCb to remove the need for AlgExecState update calls in its scheduler and circumvent sysExecute entirely.

cc: @nnolte @clemenci

changed title from New execute method to Algorithm base class to Add new execute method to Algorithm base class

changed the description

added 1 commit

• 6cebe625 - add a evaluate method to algorithm base class which returns filterPassed as...

Compare with previous version

marked as a Work In Progress

2500 dummy algorithms (one read, one write to tes) -> factor 4 improvement (this just improves further the more algorithms we have)

added 1 commit

• a1fe85eb - Fixed formatting

Compare with previous version

From an architectural point of view, adding a new method to Algorithm that is not reflected in LegacyAlgorithm, or circumvents parts of the design (ie, keeping the AlgExecState information up to date), is not a good solution to this problem. How do you enforce keeping that AlgExecState current if a user decides to use the evaluate method instead of execute? This information is essential to proper operation of other parts of the framework. LHCb is not the only one who uses the reentrant Algorithm base class - all new ATLAS Algorithms do, and there is a strong effort to migrate as many old ones as possible as well.

It seems to me that better approaches would be to optimize the AlgExecStateSvc for your use case, or return extra information via the extension to the StatusCode that Frank developed.

Since this is something that is entirely LHCb specific, it could be better to implement it in a Brunel package. Just like ATLAS has AthAlgorithm, LHCb can have its LHCbAlgorithm which will implement the evaluate method, and the LHCb scheduler can talk directly to it.

The AlgExecStateSvc was designed to remove state information from the reentrant Alg base class, which was first an LHCb requirement ;-}

I agree that we don't need the enabled/disabled checking for every event when using the scheduler. However, we sill need backward compatibility for serial usage without the scheduler, so I don't think we can get rid of this yet. Pity.

It seems that the best way to optimize performance for LHCb would be to inherit only from IAlgorithm, and have the complete implementation of LHCbAlgorithm in Brunel. This way you can get rid of or not implement all the methods you don't need. Or you can just override sysExecute to do whatever you want instead of calling AlgExecStateSvc.

At least checking `am I enabled' is just directly accessing a simple bool at a known location... so while I find it annoying as it is done 'in the wrong place', and thus makes things less clear (improper separation of responsabilities), in practice it doesn't hurt so much...

added 1 commit

• c321e18f - change functional base classes to use new evaluate method.

Compare with previous version

added 1 commit

• 348270fa - Fixed formatting

Compare with previous version

So there are 2 issues:

• the AlgExecStateSvc is too slow for your needs
• you need to get the setFilterPassed flag out of execute

Why not change Algorithm::execState to not use the AlgExecStateSvc if it's not wanted/instantiated. Since it returns a ref you'll need to be a bit tricky - maybe add a UnusedState that turns all setState calls into a noop. Then return the setFilterPassed flag as an extension to the StatusCode.

mentioned in merge request lhcb/LHCb!2171 (merged)

added 1 commit

• c0999436 - extend statuscode instead of new evaluate method

Compare with previous version

added 1 commit

• 7cd997cd - implement evaluate method via inheritance from special base class

Compare with previous version

changed title from WIP: Add new execute method to Algorithm base class to WIP: Add new execute method to functional algorithms

added 1 commit

• 1b619731 - Fixed formatting

Compare with previous version

added 1 commit

• 3e297991 - Use StatusCode extension to return filter passed or failed from functional algorithms

Compare with previous version

added 1 commit

• 75f1760b - Fixed formatting

Compare with previous version

changed the description

resolved all threads

changed title from WIP: Add new execute method to functional algorithms to WIP: Add new StatusCode category to return filterpassed states from functional algorithms

unmarked as a Work In Progress

• [2019-10-15 12:27] Validation started with lhcb-gaudi-merge#833

Hi @leggett @fwinkl,
as discussed this now uses the StatusCode to return the FilterPassed decision.
Hope this is more to your liking ;)

added 1 commit

• f50800ba - change naming of new FilterDecision statuscode

Compare with previous version

added 1 commit

• 89d751cc - Fixed formatting

Compare with previous version

added 1 commit

• 78e6d600 - update string representation

Compare with previous version

changed milestone to %v33r0

resolved all threads

resolved all threads

@fwinkl, @leggett if there are no outstanding comments anymore, could we maybe get the required approvals? :)

OK from my side. Although I don't have GitLab approval rights.

approved this merge request

approved this merge request

resolved all threads

assigned to @clemenci

added Test on LHCb label

removed Test on LHCb label

added Test on LHCb label

• [2019-10-17 17:57] Validation started with lhcb-gaudi-merge#842
• [2019-10-19 00:12] Validation started with lhcb-gaudi-head#2422
• [2019-10-20 00:07] Validation started with lhcb-gaudi-head#2423
• [2019-10-21 00:07] Validation started with lhcb-gaudi-head#2424
• [2019-10-21 13:30] Validation started with lhcb-gaudi-head#2271
• [2019-10-21 17:01] Validation started with lhcb-gaudi-head#2425
• [2019-10-22 00:09] Validation started with lhcb-gaudi-head#2426

added lhcb-gaudi-head label and removed Test on LHCb label

@clemenci this looks ok for LHCb, please go ahead (some refs need to be updated in Moore, but that'll be done today)

merged