Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace current configuration validation with a py3 compatible one #5004

Closed
belforte opened this issue May 10, 2021 · 6 comments
Closed

Replace current configuration validation with a py3 compatible one #5004

belforte opened this issue May 10, 2021 · 6 comments

Comments

@belforte
Copy link
Member

CRABClient has a sophisticated "agnostic client" where all possible parameters in crabConfig.py are described in https://github.com/dmwm/CRABClient/blob/master/src/python/CRABClient/ClientMapping.py and then checked at run time. This relies on a little known feature of types module which I could not find in python3. I worked around it by simply commenting out the current configuration validation. I propose to stick to that, and do something much simpler where we e.g. use isinstance and check all config. params one by one. A bit tedious, but clear and safe and a chance to review old things which we may possibly drop

@belforte
Copy link
Member Author

Since code works, I will live with this thing commented out and see what happens.
Maybe as simple as "when someone makes a typo in crabConfig the error may not be super-clear"

At some point we should try to pass buggy config to the py2/3 compatible client... once we have one.

@belforte belforte removed the On Hold label Dec 9, 2021
@belforte
Copy link
Member Author

belforte commented Dec 9, 2021

we have the final client now. Time to give a look at this.
I tested the "simple, stupid mistake" if putting int he config file

config.JobType.maxJobRuntimeMin = '30'

instead of

config.JobType.maxJobRuntimeMin = 30

and it "simply worked" !!

It is very difficult that users do the other thing of typing an int where a string was needed, which leads to a more obscure

belforte@lxplus738/LOCAL> crab submit 
Will use CRAB configuration file crabConfig.py
ERROR: AttributeError: 'int' object has no attribute 'rfind'

	Please email [email protected] for support with the crab.log file or crab.log URL.
	Client Version: development
	Please use 'crab uploadlog' to upload the log file /afs/cern.ch/work/b/belforte/CRAB3/TC3/LOCAL/crab_20211209_183636/crab.log to the CRAB cache.
belforte@lxplus738/LOCAL> 

looking in the log file users will find

DEBUG 2021-12-09 17:36:40.814 UTC: 	 self.config.JobType.psetName: 33
ERROR 2021-12-09 17:36:40.817 UTC: 	 Unhandled Exception!
ERROR 2021-12-09 17:36:40.818 UTC: 	 'int' object has no attribute 'rfind'
Traceback (most recent call last):

but it is hard to spot in the log noise. So something may be worth doing here (even if there's no record of a user having ever made such a mistake !).

OTOH since the configuration file is executed, most other errors like missing quotes and similar will result in clear errors anyhow.

Let's see if I find some other way to break it.

@belforte
Copy link
Member Author

belforte commented Dec 9, 2021

I tried a more likely error

config.Site.whitelist = 'T3_US_*'

in place of

config.Site.whitelist = '[T3_US_*]'

but that is clearly reported as

The server answered with an error.
Server answered with: Invalid input parameter
Reason is: Incorrect 'Site.whitelist' parameter <class 'str'> T

@belforte
Copy link
Member Author

belforte commented Dec 9, 2021

I lean to "leave things as they are"

@belforte
Copy link
Member Author

belforte commented Dec 10, 2021

for the record, the "skipping of configuration validation" was done via this commit
357b616
Screenshot from 2021-12-10 16-00-13

@belforte
Copy link
Member Author

let's simply add some comments in the code pointing to this discussion and leave it to future developers to look into making the full config. validation work again in case it is felt important.

belforte added a commit to belforte/CRABClient that referenced this issue Dec 10, 2021
@belforte belforte added Done and removed In Progress labels Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant