Adding a check for ParentageResolved before announcement

[todor-ivanov]

Fixes #545

Status

not-tested

Description

Adding a check for ParentageResolved before announcement

Is it backward compatible (if not, which system it affects?)

YES

Related PRs

dmwm/WMCore#9657

External dependencies / deployment changes

No

Mention people to look at PRs

@vlimant @sharad1126 @amaltaro

[todor-ivanov]

@sharad1126 , Can you please test this one too, and merge it at your convenience. It should not break the closor module, but anyway in the worse case scenario you have the flag in UnifiedConfig to switch it on/off. Thanks in advance.
@vlimant Do you know of another place where we should make a similar check before announcing, or all workflow announcements happen only at the closor module? Thanks

[vlimant]

all announcements are made through closor

[todor-ivanov]

@vlimant Thanks!

[sharad1126]

@todor-ivanov I don't understand the use of variable check_parentage_to_announce as it is always true so why do you even need it?

Why don't you just keep ?

if wfi.request['RequestType'] == 'StepChain':

[todor-ivanov]

@sharad1126 It is not always True, It is configurable through Unified config. You may put it to False there if you wish. AFAIK this is the way how Unified is configured. I didn't want to push few lines of code in the middle of the module which may start breaking things silently somewhere down the chain, and at the same time leaving you without any configurable parameter to switch it off. And it is True for the time being, because we need it.

[sharad1126]

@todor-ivanov I don't think it makes sense to do that. It would have been better to probably put that as an option in the module(default being true) as we can put it to false when we run the script like python Unified/closor.py --parentage_disabled. @z4027163 what's your suggestion?

[todor-ivanov]

@sharad1126, Now I do not get it .....What is the difference between configure something through a module runtime parameter and through a config file parameter?

[sharad1126]

@todor-ivanov it is simple that when you always want it to be true either you don't put the parameter at all or at a place where we can easily change it while running the script. Why do you want one to change a separate file(UC.json) just for a simple parameter that no one even cares about from what I can see. This should be just a run-time config and not change in a separate file.

[todor-ivanov]

@sharad1126 I do not know who cares about it or not. But there seemed to be a pattern on how those things were configured and I followed that. The code change is much simpler and less error prone when reading a single value from a config file, than plugging another value in the parameter parser. Something more - Now, lets assume there was another module which also deals with announcement, (Which of course was clarified later in the comments, but anyway), then we would have to plug more parameters to that one too.

[sharad1126]

I leave on @z4027163 to comment here and then follow up.

[amaltaro]

Thanks Todor. Changes look good from my side.

[amaltaro]

Now that CMSWEB upgrade is behind us, can this PR get merged such that we can mitigate the amount of StepChain workflows getting announced with incomplete parentage information?