search

cms-bril / brilws

MIT License
1 stars 3 forks source link

Adding a file exists check #7

Closed vlukashenko closed 2 months ago

vlukashenko commented 2 months ago

This MR is inspired by running into this error twice within a month: https://cms-talk.web.cern.ch/t/valueerror-malformed-node-or-string-on-line-1-when-running-brilcalc/46454 This error is triggered if the json file from crab report passed to brilcalc lumi via the '-i' argument is missing.

The python error is rather confusing so I propose to catch the error beforehand with a more human-friendly message.

I am not entirely sure how to test this PR. I have managed to successfully build the brilcalc package locally but I am not entirely sure how to easily run it from the command line (not an expert in python development). Please let me know which tests need to be performed.

xiezhen commented 2 months ago

Hello, the mentioned error messages indicate your installation environment problem. It happens before the parsing of '-i' argument. I don't see this check is useful.

xiezhen commented 2 months ago

In addition, '-i' option is designed to accept non-file argument , e.g. a json string, for quick checking . As described the full documentation. Adding this check violates the design goal.

vlukashenko commented 2 months ago

Hello, the mentioned error messages indicate your installation environment problem. It happens before the parsing of '-i' argument. I don't see this check is useful.

Hi! I do not believe it is my environment problem. I use a standard docker container on lxplus and I can reproduce this error when I pass the argument to the non-existing file. As well as, I can fix it by passing the right path. The native error is confusing and misleading, which is why it would be better to intercept the ill-defined argument before it gets passed further. I might be wrong, but then you could kindly check it with your installation if you can reproduce it. You just need to pass the non-existing json file.

Thank you for pointing out that it is not just used for the file - I have to improve the condition of the check. In any case, checking the existence of the file when the argument is a file seems like a very simple and useful check to add