Fix partitioner config exception message

[matthewrmshin]

Improve logic to eliminate config exception message. This change uses the return bool value of p.get to detect success/failure.

In a downstream application, we have been seeing error messages like these (but the messages don't result in failures):

Exception: Bad operator: cubedsphere (String) method 'contains' not implemented ...

This change fixes this. I no longer see these error messages in the downstream application.

[matthewrmshin]

@wdeconinck What do you think?

[wdeconinck]

Hi @matthewrmshin I would prefer this were not changed as your suggested change would be a change in behaviour in case the configuration contains "partitioner.type = ..." instead of "partitioner = ..." The fact the exception is thrown is a limitation that I can't work around without changing something in eckit. I will consider following this up.

As an alternative you can also silence the exception by setting the environment variable: ECKIT_EXCEPTION_IS_SILENT=1 If this is the default for your application you could do this programatically with setenv

[phlndrwd]

Thank you @wdeconinck. It's good to know this conversation is being had. This is something we need to fix, and I wouldn't say that suppressing exception messages is the way to go - we wouldn't want to miss anything that requires our attention.

I can see that @matthewrmshin has started the search for a permanent solution in Atlas itself, but you (@wdeconinck) seem to be saying that the current implementation is the best available under current constraints. I've attempted to approach this from our model interface in LFRic-Lite (https://github.com/JCSDA-internal/lfric-lite-jedi). However, I cannot find a configuration what works. The obvious place to start is to change the string in this line: https://github.com/JCSDA-internal/lfric-lite-jedi/blob/84cc661c9180540216b49e3789c136be7f8957b1/src/lfriclitejedi/Geometry/Geometry.cc#L60 from "partitioner" to "partitioner.type". However this generates an error:

...failed with unhandled eckit::Exception: Bad Conversion: Cannot convert {type => cubedsphere} (OrderedMap) to std::string...

I'm wondering if "partitioner.type" is a reference to some kind of sub-configuration setup. Can you please point me to a piece of code that shows the intended use-case for this code, @wdeconinck?

Cheers!

[matthewrmshin]

Just trying to get my head around the original logic:

    try { p.get("partitioner.type", partitioner); } catch( std::exception& ) {}
    p.get("partitioner", partitioner);

Looking at the API of p which is a eckit::Parametrisation type, the p.get method with two std::string& arguments does not throw an exception when the value of the name in the first argument is not found in the data structure. It simply returns the boolean false. Even if it does throw an exception, the exception will be caught by the catch( std::exception& ) and handled by the empty {} block.

In both cases, execution will always continue to the next line p.get("partitioner", partitioner);, so it looks to me that the partitioner variable will always be set to the value of partitioner as long as partitioner exists (which should include the case when partitioner.type exists - as partitioner.type cannot be exist without partitioner being there).

So I believe my original fix is actually the closest to the original logic. Please feel free to change my mind. 😅

[odlomax]

Just trying to get my head around the original logic:

    try { p.get("partitioner.type", partitioner); } catch( std::exception& ) {}
    p.get("partitioner", partitioner);

Looking at the API of p which is a eckit::Parametrisation type, the p.get method with two std::string& arguments does not throw an exception when the value of the name in the first argument is not found in the data structure. It simply returns the boolean false. Even if it does throw an exception, the exception will be caught by the catch( std::exception& ) and handled by the empty {} block.

In both cases, execution will always continue to the next line p.get("partitioner", partitioner);, so it looks to me that the partitioner variable will always be set to the value of partitioner as long as partitioner exists (which should include the case when partitioner.type exists - as partitioner.type cannot be exist without partitioner being there).

So I believe my original fix is actually the closest to the original logic. Please feel free to change my mind. 😅

I think the problem is that the config object p is of type eckit::Parameterisation. It has a has method, but that only checks for the existence of a key. We can check for a string via get("key", strObject), but it cannot do the same for an eckit::LocalConfiguration, which, as far as I can tell, is the type of the new "partitioner" object. Calling p.get("partitioner.type", partitioner), when the "partitioner" object in the config is a string, will throw an exception.

If you downcast &p to a const eckit::Configuration*, we can use get("key", configObject), which allows us to safely see if there's a config object keyed to "partitioner".

The downside of this, of course, is that it's really ugly.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (e734e7a) 79.96% compared to head (0de5733) 80.02%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #167 +/- ## =========================================== + Coverage 79.96% 80.02% +0.06% =========================================== Files 839 857 +18 Lines 63246 63494 +248 =========================================== + Hits 50573 50813 +240 - Misses 12673 12681 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.