Fix table-related error messages #3524

verveerpj · 2023-05-15T05:52:25Z

This PR fixes missing location information in the table-related code

verveerpj · 2023-05-15T06:09:33Z

I noted something else, and would like some feedback on that:

The table-related errors that are handled here are printed twice to the console, and this is likely also occurring to other errors that are raised by throwing OpmInputError. The errors are printed the first time by the following code in opm-common:

src/opm/input/eclipse/EclipseState/EclipseState.cpp

Here the exception is caught, printed and thrown again. It is then caught and printed again, after reading the deck, in opm-simulators:

opm/simulators/utils/readDeck.cpp

My intuition would be that the error should not be caught, printed and thrown again in the first piece of code, and only be printed after reading the deck. However, I would hesitate to remove the first printing it in fear of loosing the error reporting in other code.

alfbr · 2023-05-15T07:58:31Z

I have noticed the double printing of messages, and it seems you have identified the culprit. Removing it is very welcome from my end. If nobody can clarify your concern about removing too much, I am happy to be a test runner for checking it.

verveerpj · 2023-05-15T09:11:13Z

I have noticed the double printing of messages, and it seems you have identified the culprit. Removing it is very welcome from my end. If nobody can clarify your concern about removing too much, I am happy to be a test runner for checking it.

Then I propose to wait shortly if anybody else has input on this. Meanwhile I can add a commit to remove the double printing to test if that breaks anything immediately.

alfbr · 2023-05-15T10:12:21Z

Sounds like a plan :)

verveerpj · 2023-05-15T14:52:58Z

jenkins build this please

verveerpj · 2023-05-15T15:19:06Z

jenkins build this please

Do I need to be white-listed for this?

goncalvesmachadoc · 2023-05-17T09:34:05Z

jenkins build this please

bska

I think I'd personally prefer not adding the colons to the diagnostic messages, but that aside I'm a little concerned about the revised exception handling here. I think we should do a careful review before making the changes proposed here.

bska · 2023-05-18T17:26:46Z

src/opm/input/eclipse/EclipseState/EclipseState.cpp

-    catch (const OpmInputError& opm_error) {
-        OpmLog::error(opm_error.what());
-        throw;
-    } 
    catch (const std::exception& std_error) {


Class OpmInputError inherits from std::exception. Removing the OpmInputError clause will just mean that we end up in the std::exception clause instead. That's unlikely to do any good, so we'll have to rethink this approach.

You are correct, the wrong fix ended up in the commit, the intention was to only remove the logging call. Fixed that.

bska · 2023-05-18T17:30:24Z

src/opm/input/eclipse/EclipseState/Tables/TableManager.cpp

-                } catch (const std::runtime_error& err) {
+                } catch (const std::invalid_argument& err) {


I don't think this is a valid transformation. You'll have to ensure that any exceptions thrown from the TableType constructor (or the addTable() function call, for that matter) that are not invalid_argument can safely propagate to the caller. Unless you've already done that I'd be wary to do this.

I am not very familiar with this code, but I assumed the intention of this particular snippet was to transform errors reported in the table code to add location information. Since the table code uses invalid_argument exceptions those would need to be caught and not runtime_error which is not thrown there afaik.

If there is a need is to add location information to a broader spectrum of exceptions, then these need to caught also, but it is not clear to me what exceptions would need to be caught. We could catch std::exception to turn anything into OpmInputError, but I am not that familiar with OPM error handling, not sure if that is desirable.

verveerpj · 2023-05-19T07:17:00Z

I think I'd personally prefer not adding the colons to the diagnostic messages

That is fair, and I do not mind either way. In this case I tried to harmonize the formatting of the error messages. If somebody makes the decision on how we want it, I will fix to abide.

verveerpj · 2023-05-19T07:20:49Z

I'm a little concerned about the revised exception handling here. I think we should do a careful review before making the changes proposed here.

I agree, that is why I indicated initially that I am not sure if this may have unintended consequences. Removing this logging call may lead to lost error messages in another context. Alternative would be to leave it and not report in readDeck.cpp. I am not sure if that is the right approach, I am not sufficiently familiar with the error handling philosophy of OPM, do we report "on the go" (i.e., this log statement needs to stay and the one in readDeck.cpp removed, or "at the end" (i.e. as I suggested in this commit)?

goncalvesmachadoc · 2023-05-31T06:00:29Z

I think I'd personally prefer not adding the colons to the diagnostic messages

That is fair, and I do not mind either way. In this case I tried to harmonize the formatting of the error messages. If somebody makes the decision on how we want it, I will fix to abide.

I think is good to be consistent with the other error messages. If we decide to change the style, we could do it in a later PR for all error messages, not only this table related errors.

goncalvesmachadoc · 2023-05-31T06:07:27Z

I'm a little concerned about the revised exception handling here. I think we should do a careful review before making the changes proposed here.

I agree, that is why I indicated initially that I am not sure if this may have unintended consequences. Removing this logging call may lead to lost error messages in another context. Alternative would be to leave it and not report in readDeck.cpp. I am not sure if that is the right approach, I am not sufficiently familiar with the error handling philosophy of OPM, do we report "on the go" (i.e., this log statement needs to stay and the one in readDeck.cpp removed, or "at the end" (i.e. as I suggested in this commit)?

Maybe you could make a separate PR with the removal of double printed messages to be further tested? Meanwhile we could already get the error location information that are relevant for the users in.

verveerpj · 2023-05-31T09:17:57Z

I think I'd personally prefer not adding the colons to the diagnostic messages

That is fair, and I do not mind either way. In this case I tried to harmonize the formatting of the error messages. If somebody makes the decision on how we want it, I will fix to abide.

I think is good to be consistent with the other error messages. If we decide to change the style, we could do it in a later PR for all error messages, not only this table related errors.

The changes apply to the printing of all errors thrown via OpmInputError, so they will apply broadly, increasing overall consistency. But it might indeed be better to make a separate PR for it.

verveerpj · 2023-05-31T09:18:55Z

I'm a little concerned about the revised exception handling here. I think we should do a careful review before making the changes proposed here.

I agree, that is why I indicated initially that I am not sure if this may have unintended consequences. Removing this logging call may lead to lost error messages in another context. Alternative would be to leave it and not report in readDeck.cpp. I am not sure if that is the right approach, I am not sufficiently familiar with the error handling philosophy of OPM, do we report "on the go" (i.e., this log statement needs to stay and the one in readDeck.cpp removed, or "at the end" (i.e. as I suggested in this commit)?

Maybe you could make a separate PR with the removal of double printed messages to be further tested? Meanwhile we could already get the error location information that are relevant for the users in.

Yes, that seems like a good idea.

bska · 2023-06-01T16:58:09Z

I have noticed the double printing of messages, and it seems you have identified the culprit. Removing it is very welcome from my end. If nobody can clarify your concern about removing too much, I am happy to be a test runner for checking it.

Meanwhile I can add a commit to remove the double printing to test if that breaks anything immediately.

For what it's worth, Issue #3239 also notes the "double printing" problem.

verveerpj · 2023-06-02T10:39:46Z

I have removed the commits that handle formatting and double printing. I will make separate PRs for those.

This PR now only handles the error handling for the table related error messages. It does that by adding a catch block for transforming invalid_argument exceptions to OpmInputError exceptions. That resolves the issue of missing location information for the table related errors as far as I can see.

Any other exception is let to propagate as before, and will be handled eventually, just without location information. Those could be included by catching a broader spectrum of exceptions, but I am not sure which those should be and I am wary of catching all exceptions. I would suggest taking action when that occurs and more errors are observed without location information.

goncalvesmachadoc · 2023-06-04T21:19:25Z

jenkins build this please

blattms · 2023-07-05T12:22:55Z

jenkins build this please

blattms · 2023-07-05T13:12:54Z

thanks a lot. Having line and file information there really helps a lot. Without it the error is rather useless. Merging.

verveerpj self-assigned this May 15, 2023

verveerpj force-pushed the fix-table-error-msgs branch from e6f5713 to aae6284 Compare May 15, 2023 14:51

bska reviewed May 18, 2023

View reviewed changes

verveerpj force-pushed the fix-table-error-msgs branch 2 times, most recently from e30aa62 to a89313d Compare May 19, 2023 07:14

plgbrts mentioned this pull request May 23, 2023

adding defaults to json files #3465

Merged

verveerpj force-pushed the fix-table-error-msgs branch from a89313d to 6842257 Compare June 2, 2023 10:20

verveerpj force-pushed the fix-table-error-msgs branch from 1d4cbac to 3ee4792 Compare June 2, 2023 10:43

Fix table related error messages

fb4514a

verveerpj force-pushed the fix-table-error-msgs branch from 3ee4792 to fb4514a Compare June 13, 2023 09:22

blattms merged commit 98fe4e4 into OPM:master Jul 5, 2023

verveerpj deleted the fix-table-error-msgs branch December 18, 2023 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix table-related error messages #3524

Fix table-related error messages #3524

verveerpj commented May 15, 2023 •

edited

Loading

verveerpj commented May 15, 2023 •

edited by bska

Loading

alfbr commented May 15, 2023

verveerpj commented May 15, 2023

alfbr commented May 15, 2023

verveerpj commented May 15, 2023

verveerpj commented May 15, 2023

goncalvesmachadoc commented May 17, 2023

bska left a comment

bska May 18, 2023

verveerpj May 19, 2023

bska May 18, 2023

verveerpj May 19, 2023

verveerpj commented May 19, 2023

verveerpj commented May 19, 2023

goncalvesmachadoc commented May 31, 2023

goncalvesmachadoc commented May 31, 2023

verveerpj commented May 31, 2023

verveerpj commented May 31, 2023

bska commented Jun 1, 2023

verveerpj commented Jun 2, 2023

goncalvesmachadoc commented Jun 4, 2023

blattms commented Jul 5, 2023

blattms commented Jul 5, 2023

		} catch (const std::runtime_error& err) {
		} catch (const std::invalid_argument& err) {

Fix table-related error messages #3524

Fix table-related error messages #3524

Conversation

verveerpj commented May 15, 2023 • edited Loading

verveerpj commented May 15, 2023 • edited by bska Loading

alfbr commented May 15, 2023

verveerpj commented May 15, 2023

alfbr commented May 15, 2023

verveerpj commented May 15, 2023

verveerpj commented May 15, 2023

goncalvesmachadoc commented May 17, 2023

bska left a comment

Choose a reason for hiding this comment

bska May 18, 2023

Choose a reason for hiding this comment

verveerpj May 19, 2023

Choose a reason for hiding this comment

bska May 18, 2023

Choose a reason for hiding this comment

verveerpj May 19, 2023

Choose a reason for hiding this comment

verveerpj commented May 19, 2023

verveerpj commented May 19, 2023

goncalvesmachadoc commented May 31, 2023

goncalvesmachadoc commented May 31, 2023

verveerpj commented May 31, 2023

verveerpj commented May 31, 2023

bska commented Jun 1, 2023

verveerpj commented Jun 2, 2023

goncalvesmachadoc commented Jun 4, 2023

blattms commented Jul 5, 2023

blattms commented Jul 5, 2023

verveerpj commented May 15, 2023 •

edited

Loading

verveerpj commented May 15, 2023 •

edited by bska

Loading