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
Allow throwing of edm::Exceptions from python #40315
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40315/33395
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a unit test?
@@ -39,6 +40,8 @@ PYBIND11_MODULE(libFWCorePythonParameterSet, m) { | |||
std::rethrow_exception(p); | |||
} catch (const cms::Exception &e) { | |||
PyErr_SetString(PyExc_RuntimeError, e.what()); | |||
} catch (const pybind11::error_already_set &e) { | |||
std::cout << "pybind " << e.what() << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this printout still useful?
auto errorNameEnd = error.find('}'); | ||
if (errorNameStart != std::string::npos and errorNameEnd != std::string::npos) { | ||
auto errorName = error.substr(errorNameStart + 1, errorNameEnd - errorNameStart - 1); | ||
std::cout << "EDMException '" << errorName << "'" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this printout be removed?
auto errorName = error.substr(errorNameStart + 1, errorNameEnd - errorNameStart - 1); | ||
std::cout << "EDMException '" << errorName << "'" << std::endl; | ||
if ("Configuration" == errorName) { | ||
std::cout << "throwing edm::Exception" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this printout be removed?
if (errorNameStart != std::string::npos and errorNameEnd != std::string::npos) { | ||
auto errorName = error.substr(errorNameStart + 1, errorNameEnd - errorNameStart - 1); | ||
std::cout << "EDMException '" << errorName << "'" << std::endl; | ||
if ("Configuration" == errorName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be feasible to generalize the name-to-error code translation by exploiting the information in
cmssw/FWCore/Utilities/src/EDMException.cc
Lines 9 to 10 in 52c0dac
static const std::map<ErrorCodes, std::string> codeMap = {FILLENTRY(CommandLineProcessing), | |
FILLENTRY(ConfigFileNotFound), |
?
On the other hand, the number of error codes to throw from the python code is likely small (I can think of one to add, but not really more)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to only allow a very select few edm.errors
to be used.
FWCore/ParameterSet/python/Config.py
Outdated
Configuration = "{Configuration}" | ||
|
||
class EDMException(Exception): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not crystal clear to me how these would be used. Based on the implementation I'd guess
raise EDMException(edm.error.Configuration + "Real exception message")
How about something along the following interface?
raise EDMException(edm.error.Configuration, "Real exception message")
@@ -240,6 +240,8 @@ int main(int argc, char* argv[]) { | |||
try { | |||
std::unique_ptr<edm::ParameterSet> parameterSet = edm::readConfig(fileName, argc, argv); | |||
processDesc.reset(new edm::ProcessDesc(std::move(parameterSet))); | |||
} catch (edm::Exception const&) { | |||
throw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if adding the calling context to the exception would be helpful to interpret the exception message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call actually does have a lot of context. E.g.
----- Begin Fatal Exception 14-Dec-2022 14:21:44 CST-----------------------
An exception of category 'Configuration' occurred while
[0] Processing the python configuration file named test_pyhon_error.py
Exception Message:
test message
At:
test_pyhon_error.py(11): <module>
----- End Fatal Exception -------------------------------------------------
for my trivial test case where the configuration file only has the raise
call.
Only a selected set of edm::errors codes are allowed.
1b629a1
to
6e6d795
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40315/33396
|
Pull request #40315 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-baccdf/29659/summary.html Comparison SummarySummary:
|
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Only a selected set of edm::errors codes are allowed.
PR validation:
Code compiles and a simple script shows it works.