search

dmwm / CRABClient

runrange
14 stars 35 forks source link

optional string configuration param should default to None, not `''` #5309

Closed belforte closed 2 months ago

belforte commented 2 months ago

this will allow "sane" validation in the REST, see e.g. https://github.com/dmwm/CRABServer/issues/8372#issuecomment-2106380644

I suspect that current code is a legacy of (very old) thing of trying to validate parameter type in the client, and some mis-conception/mis-comunication about what required=False should mean.

Of course need to test that nothing breaks with this change

belforte commented 2 months ago

all standard tests worked https://github.com/dmwm/CRABServer/issues/8400 But I do not feel fully confident yet.

belforte commented 2 months ago

I verified that with this change when config.Data.inputDataset is not present in crabConfig.py the URL used for submission does not have &inputdata=& but simply inputdata argument is not there. Which is the desired behavior.

belforte commented 2 months ago

Trying to think of what else to test.

belforte commented 2 months ago

testing StatusTracking as well: https://github.com/dmwm/CRABServer/issues/8403

hmm.. submission of privateMC to preprod with this client failed, but it worked on my (modified) test2. Investigating. OK. in my earlier test I used un-modified client. Problems is clear in https://github.com/dmwm/CRABServer/blob/c69980662cbd4fa76e3801b6f3085856032743d6/src/python/CRABInterface/RESTUserWorkflow.py#L345

                if param.kwargs['inputdata'] or 'inputblocks' in param.kwargs:
belforte commented 2 months ago

so this will be a 3-step process:

  1. change REST to accept new URL
  2. change Client not to send &inputata=& in the URL
  3. change REST to use the new format to do a proper and cleaner validation of inputdata
belforte commented 2 months ago

closing again. Will remember to update REST before tagging and deploying.