search

rdkit / knime-rdkit

The RDKit nodes for the KNIME Analytics Platform
26 stars 13 forks source link

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

Open manuelschwarze opened 3 years ago

manuelschwarze commented 3 years ago

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:

https://github.com/vernalis/vernalis-knime-nodes/commit/ebc1d9f0dfd2d54c4ae67e29e40eb00117069f7c

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 commented 3 years ago

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