Validate spec run lists against integer data type #11535
Conversation
|
@amaltaro , could you please quickly look into this PR and provide me feedback if I'm moving in a right direction. If so, I'll inspect other specs and adjust PR accordingly. |
|
Jenkins results:
|
|
@vkuznet Valentin, this development is in the right direction. However, I just noticed that we overlooked something much more simple. Here is how these 2 attributes are defined in StdBase: so I think we can simply update the It needs to be properly tested though and also propagated to TaskChain/StepChain specs. |
|
test this please |
|
Jenkins results:
|
|
Jenkins results:
|
There was a problem hiding this comment.
Valentin, I don't think we need the custom validate function. The only places that we should update are the validate function in the factories:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMSpec/StdSpecs/DataProcessing.py#L48
and
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMSpec/StdSpecs/StdBase.py#L1234
of course, for both RunWhitelist and RunBlacklist.
I left a comment in the unit test as well, suggesting to do the same for TaskChain and StepChain, to ensure that the run validation will get applied to inner task/step description as well.
| @@ -52,6 +52,20 @@ def testStdBaseValidation(self): | |||
| stdBaseInstance.factoryWorkloadConstruction("TestWorkload", arguments) | |||
| return | |||
|
|
|||
| def testRunlist(self): | |||
There was a problem hiding this comment.
I would suggest a similar test for the TaskChain and StepChain factories.
|
Jenkins results:
|
|
Jenkins results:
|
There was a problem hiding this comment.
@vkuznet Valentin, it could be that you missed my previous review. I suggested to completely get rid of the validateRunlist custom validate function.
I think the lambda functions in the spec will do everything that we need to be done. However, I'd suggest to ensure that this is tested in a TaskChain and StepChain unit test.
When you change it, please initially provide a new commit though (don't squash for now), just so we can make sure it's not really needed.
|
Alan, I did not miss your review, and intentionally left |
|
Okay, here is what I would have to do to confirm the behavior: Instead of me making all of this, I would suggest you to give it a try and learn how to test this code and avoid surprises in production. In addition, please update the PR title with a more meaningful message (I didn't check the commit, but if needed, update the commit message as well). |
|
@amaltaro , I performed approach (a) and all StdSpec_t in docker image run fine. For approach (b) I can patch reqmgr2 pod on test9 cluster but I have no idea how to locate and run templates for such test. Please provide concrete set of instruction if you think it will be necessary. |
|
Sure, here is a wiki on injecting and checking/validating test workflows: Note-1: you do not really need to run any workflows, you only want to test their creation in ReqMgr2 (which is when the validation should kick in). |
|
Alan, the wiki is far from clear. Now, we have installation of WMCore through pypi and it install stuff into |
|
Templates are available under the DMWM and Integration directories in: and they are downloaded automatically whenever you execute the inject.. script. You definitely do not need to be in your central services to run it. You only need your grid credentials and download the script (well, and python3). |
|
Alan, I patched the reqmgr2 pod in test9 cluster and run the following injection where I adjusted But, somehow the injection script skip the template I provided. What's wrong? |
|
We skip those templates because those workflows don't run to the completion (lack of parent data in workqueue). You can modify this line: To test a StepChain template, you can inject this one: which btw, will also be skipped by the script unless you modify it. |
|
Alan, I modified injection script, setup proxy but run into SSL issue. Here is full output: It seems to me that reqmgr2 pod lacks of something which does not allow me to proceed with our proxy file. Please note we deploy to k8s proxy using robot certs. Said that, I do not know how proxy in your test9 cluster is made and is it valid or not. I suggest that you have a look into this issue. For the record I use /tmp area where I downloaded the injection script and WMCore and the pod name is |
|
@vkuznet Valentin, it could be that there is an openssl incompatibility between the node you are using and the server. In any case, here is my terminal output injecting that template with my own proxy: |
|
@vkuznet Valentin, I managed to test a ReReco, TaskChain and StepChain "bad" workflow and all of them failed with the same error message, e.g.: which means that the StdBase/DataProcessing spec level Could you please clean up this PR by removing the extra function for validating these run lists? |
vkuznet
force-pushed
the
fix-issue-11466
branch
from
360f644
to
36f5cf5
Compare
|
Jenkins results:
|
|
Alan, I removed |
| @@ -46,9 +46,9 @@ def getWorkloadCreateArgs(): | |||
| "PrimaryDataset": {"optional": True, "validate": primdataset, | |||
| "attr": "inputPrimaryDataset", "null": True}, | |||
| "RunBlacklist": {"default": [], "type": makeList, "null": False, | |||
| "validate": lambda x: all([int(y) > 0 for y in x])}, | |||
| "validate": lambda x: all([isinstance(y, int) for y in x])}, | |||
There was a problem hiding this comment.
Valentin, I think we should actually keep the old validation as well. So the code that I would suggest to have in place is isinstance(y, int) and int(y) > 0, thus catching either the data type and a positive number.
vkuznet
force-pushed
the
fix-issue-11466
branch
from
36f5cf5
to
64012b3
Compare
|
good point, I added the positive number check back. |
|
Jenkins results:
|
amaltaro
changed the title
Fixes #11466
Status
In development
Description
Provide validation function for RunWhite/Blacklist.
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
External dependencies / deployment changes