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!

[odlomax]

At the risk of increased verbosity, but less exception scariness, how about replacing 66 and 67 with:

    auto partConf = eckit::LocalConfiguration{};
    const auto* pPtr = dynamic_cast<const eckit::Configuration*>(&p);
    if (pPtr && pPtr->get("partitioner", partConf)) {
      partConf.get("type", partitioner);
    } else {
      p.get("partioner", partitioner);
    }

Edit: replaced static cast with dynamic equivalent.

[wdeconinck]

I believe I have another proposal that should work instead:

    // Get partitioner.
    if (p.has("partitioner.type")) {
        std::string partitioner;
        p.get("partitioner.type", partitioner);
        options.set("partitioner", partitioner);
    }
    else if (p.has("partitioner")) {
        std::string partitioner;
        p.get("partitioner", partitioner);
        options.set("partitioner", partitioner);
    }

Could you test this change instead and get back to me if that also solves the issue?

[odlomax]

That looks far more civilised than juggling pointers...

[phlndrwd]

Thank you, @wdeconinck. I've tested this extensively. Unfortunately, it doesn't solve our problem at all. It seems that the "has" function performs something of an unsympathetic search that continues to generate the underlying exception in eckit we're trying to avoid. Removal of the try-catch, as in your suggestion therefore results in the exception terminating the running program/test.

I've found a slight modification that works for us:

    // Get partitioner.
    try {
        if (p.has("partitioner")) {
            std::string partitioner;
            p.get("partitioner", partitioner);
            options.set("partitioner", partitioner);
        }
        else if (p.has("partitioner.type")) {
            std::string partitioner;
            p.get("partitioner.type", partitioner);
            options.set("partitioner", partitioner);
        }
    } catch( std::exception& ) {}

This wraps your suggestion, @wdeconinck, in a try-catch and swaps the if clauses so that our current configuration is successfully found first and the exception is avoided. However, this will obviously generate the exception message for users who have moved to this new "partitioner.type" use-case.

Is it possible you could show us an example of this use-case being used as intended? As described above, simply swapping our call to the atlas::MeshGenerator to use the handle "partitioner.type" instead of "partitioner" (as we do now) seems to introduce a different problem. It strikes me that the other solution if it's the preferred approach, is for us to move our model interfaces to use this new use-case.

[wdeconinck]

Following not yet merged commit in eckit would make my latest suggestion work in the future.

I cannot give an existing use case for this @phlndrwd. It could have been as part of some testing in another project that I don't remember. It also does not seem to be added to the other MeshGenerators strangely. We can for now accept @matthewrmshin and reintroduce (if needed) support for "partitioner.type", which would then also require changes in other mesh generators.

[phlndrwd]

Thanks @wdeconinck! I certainly prefer a long-lasting solution and I'm happy to hear that you guys are already on it :)

[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 ✅

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

Additional details and impacted files

@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.