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

Provide better error messages when RDKit nodes fail completely or by row #83

Open
manuelschwarze opened this issue Dec 2, 2020 · 1 comment

Comments

@manuelschwarze
Copy link
Contributor

Steve Roughley, Vernalis, has suggested the following interesting idea as it was successfully implemented already in the Vernalis nodes:

In the spirit of OSS/sharing etc I thought that this latest commit to the Vernalis code might be of interest:

vernalis/vernalis-knime-nodes@ebc1d9f

In particular this file - https://github.com/vernalis/vernalis-knime-nodes/blob/master/com.vernalis.knime.chem.core/src/com/vernalis/knime/chem/rdkit/RDKitRuntimeExceptionHandler.java

Basically this provides a mechanism for handling RDKit exceptions for either the new (4.1) or older versions of the RDKit Types plugin. Typical usage:

   try{
          //Do something throwing e.g. GenericRDKitException or MolSanitizeException…

   } catch (MolSanitizeException e) {
          // Just re-throw, but message is available via standard #getMessage() call
          throw new RDKitRuntimeExceptionHandler(e);
   } catch (GenericRDKitException e) {
          // Just log the message…
          getLogger().warn(new RDKitRuntimeExceptionHandler(e).getMessage());
   }

Hope that’s of use…
Steve

I think this is a very good idea, but would require some amount of refactoring as we have to go through all RDKit nodes code and rework the error handling and automated tests - that would be a looong journey.

Steve had some other idea: Maybe we can make the #getMessage() work in the wrappers in RDKit – that would solve the problem for once and all. Then we can simply rely on having a meaningful message in all exceptions coming from RDKit.

If I understand it correctly, that would mean to improve the SWIG mechanism that creates the wrapper classes... - It will be more on the RDKit side, not so much on the RDKit Nodes side.

@sroughley
Copy link

You might be able to do this more quickly than you think. In Eclipse, if you use a Java Search for 'message' on Methods as shown then you will find everywhere you use the legacy method and need to modify:
image

If you only want the exception message then replaceing e.message() with new RDKitRuntimeExceptionHandler(e).getMessage() is enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants