New Site.acceleratorParams as json-like dictionary

[novicecpp]

Part of #6989
Client side: dmwm/CRABClient#5179

New Site.acceleratorParams configuration option to let users request specific GPU by specify GPU parameter as json-like key/value dictionary.
It is optional argument but require Site.requireAccelerator to be True.
Support CUDACapabilities, CUDARuntime, and GPUMemoryMB keys.

Note that server get json string from client and parse in new validate_dict contextmanager and expose tuple of RESTArgs back to with statement and execute key validation statement inside.

[belforte]

very nice, thanks for introducing use to the use of contextlib ! Sort of "even too careful", I am not sure that we need to validate all acceleratorparams here, but surely looks good.

I have a couple general comments:

Should we propose to add the new validate_dict method to WMCore so it stays together with other validate_* ?

I am curious if you considered to generalize this to accept an additionalParams argument to the REST which is a dictionary like `additionaParams={k1:v1, k2,v2, ...} Where REST only validates the structure as a dictionary and stores it in the DB. Then applications like TaskWorker retrieve it and look e.g. for k=acceleratorParams, or k=someOtherThings and then expand the value as needed and in case raise an exception there if format is not good. While I respect the idea of the REST/Server.py original developer to protect the REST against attacks, I am not sure that in our use case we gain. In a way making it possible to pass one more paramerter from Client to Server w/o changing the REST code has its advantages.

[novicecpp]

Should we propose to add the new validate_dict method to WMCore so it stays together with other validate_* ?

Ok. I will open PR.

I am curious if you considered to generalize this to accept an additionalParams argument to the REST which is a dictionary like `additionaParams={k1:v1, k2,v2, ...} Where REST only validates the structure as a dictionary and stores it in the DB. Then applications like TaskWorker retrieve it and look e.g. for k=acceleratorParams, or k=someOtherThings and then expand the value as needed and in case raise an exception there if format is not good. While I respect the idea of the REST/Server.py original developer to protect the REST against attacks, I am not sure that in our use case we gain. In a way making it possible to pass one more paramerter from Client to Server w/o changing the REST code has its advantages.

No.

REST.validate() does input validate/sanitize that it is safe to use anywhere in our service.

Let's say you have a new k=mynewkey and use it in DagmanCreator.py, you validate/sanitize input there. Next month, you want to use it in DagmanSubmitter.py and also put validation in there, or better put it in Utilities.py and share it between two files. But, it is similar to placing validation inside REST.

On the contrary, you want to deprecate k=myoldkey. If we do not check at the REST, the user will not get a rejected message from CRAB. Users will assume that provided parameter is still in use.

What we do to validate user input in REST is just copy/paste pattern, nothing new.

[novicecpp]

@belforte One question before opening PR to WMCore.
Should we flatten dict to individual parameters (in client)?
The Pro is we do not need to handle new dict type (no validate_dict function) on server side.
But it needs to change client code, which is not trivial. Or we can expose it as named parameters instead.

[belforte]

I think we disagree on whether it is more convenient to validate k,v pairs in REST or TW. But let's wait until we have a concrete use case for more parameters from client.

About PR for WMCore and flattening dictionary in client, the whole point here was to be able add some other params to accparams w/o changing code in Client and REST, and possibly pass them in the JDL as a classad w/o any manipulation. If that's not possible, I would question the usefulness of all this, review very critically the design and look for a different way. There is also the secondary advantage of not needing to introduce new columns in the DB. Anyhow I still think it nice to be able use a dictionary as REST arg, which your validate_dict makes it possible.

[novicecpp]

For the client, YES, I agree with you.

But for the REST, NO. You still need to validate value inside dict, and RESTUserWorkflow.validate() is the best place to do it.

• Use validate_* function to validate type/size/format.
• Take advantage of validate() machinery that will reject submissions if there is any extra parameter we did not validate, which I already implemented the same way in validate_dict.
• Validate once at the place where we receive data and "safely" use it everywhere.

Or may I ask how do you plan to validate in TW?

[belforte]

Yes, your approach is the "correct" one and in line with the original REST design. I am simply sloppy and lazy and imagine that a wrong key name or value would result in an exception down the road which will hopefully be clear in its message. Yeah.. I know, that basically mean no validation: take a json object from client and use it. I agree that it is not a safe practice. I am happy to take your code as is, as I already wrote.

Anyhow AFAIC see the plan is:

• do not flatten dict in client
• keep your validation in the REST
• possibly move validate_dict in WMCore

[novicecpp]

Thanks Stefano.

[cmsdmwmbot]

Jenkins results:

• Python3 Pylint check: failed
  • 29 warnings and errors that must be fixed
  • 10 warnings
  • 275 comments to review
• Pycodestyle check: succeeded
  • 405 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-CRABServer-PR-test/1391/artifact/artifacts/PullRequestReport.html