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

ObjectPath XML errors feedback #2341

Merged
merged 27 commits into from
Apr 25, 2023
Merged

Conversation

MelReyCG
Copy link
Contributor

This PR aims:

1. To prevents crashing when PathCollection::objectPath is wrong (thus, solving the case 2.1 of the issue #2320)

2. Add a context in case of a wrong PackCollection or FieldSpecification objectPath so we know from which object the error comes from (thus, solving the case 2.2 of the issue #2320).
In order to output the name of the Group with a wrong objectPath, this PR propose to use the GEOSX_THROW macros and try-catch to add a context to the originally thrown exception (inserting a line starting with ***** ERROR:):

***** ERROR: initialPressure has a wrong objectPath. The following error have been thrown:
***** LOCATION: /appli_PITSI/MelvinRey/GEOSX_repos/GEOSX/src/coreComponents/mesh/MeshObjectPath.cpp:225
***** Controlling expression (should be false): !foundMatch
***** Rank 0: elementRegionsGroup doesn't have a child named badRegion.
elementRegionsGroup have the following children: reservoir, wellRegion

3. To fix Group::getPath() so it no longer starts with m/ but with / (that m was the end of Problem which seemed wrongly cut). This fix will help a bit to output clearer path errors (this was the only use of getPath()). The returned path would be of the following form: /Tasks/myPackCollection (the leading / symbolizing it is an absolute path)
I added testGroupPath to test:

  • the getPath() output,
  • the getGroupByPath() input,
  • if the Group hierarchy keeps being as we expect.

@MelReyCG MelReyCG linked an issue Mar 16, 2023 that may be closed by this pull request
9 tasks
@MelReyCG MelReyCG requested a review from TotoGaz March 16, 2023 14:18
@MelReyCG MelReyCG self-assigned this Mar 16, 2023
@MelReyCG MelReyCG added type: bug Something isn't working flag: ready for review labels Mar 16, 2023
src/coreComponents/dataRepository/Group.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.cpp Outdated Show resolved Hide resolved
src/coreComponents/mesh/MeshObjectPath.cpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.cpp Outdated Show resolved Hide resolved
@MelReyCG MelReyCG requested a review from TotoGaz March 28, 2023 14:08
- Added a constructor to InputError to add text in a previously caught exception.
@MelReyCG MelReyCG requested a review from untereiner March 29, 2023 13:54
src/coreComponents/mesh/MeshObjectPath.cpp Outdated Show resolved Hide resolved
src/coreComponents/mesh/MeshObjectPath.cpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.hpp Outdated Show resolved Hide resolved
src/coreComponents/common/Logger.cpp Outdated Show resolved Hide resolved
src/coreComponents/common/Logger.cpp Outdated Show resolved Hide resolved
- GEOSX_FMT param id fix
src/coreComponents/common/Logger.cpp Show resolved Hide resolved
src/coreComponents/common/Logger.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.hpp Outdated Show resolved Hide resolved
@klevzoff
Copy link
Contributor

klevzoff commented Apr 3, 2023

Sorry to chime in uninvited and this late into the PR's lifecycle, but I find the whole approach of parsing error messages and attempting to insert text in the middle logically "backwards" and extremely brittle. If someone changes the message format in LvArray logging macro and replaces 5 stars with 4 or something, this functionality might be silently broken.

Not to stand in the way of merging this as is (it is an improvement for end users), but we should consider an overall change to how throwing macros work: instead of adding all the extra formatting (rank number, stars, etc.) at the source of throw, we should throw just the succinct error message and add all the other stuff at the end (when the message gets printed). This will allow us to easily "stack" multiple contexts (messages) line-by-line as the exception propagates up the call stack, without having to parse the previous text. We could even preserve the original exception object and wrap it in the new one that gets re-thrown (e.g. nested_exception). This would be a separate PR, if anyone has free cycles to do it.

@TotoGaz
Copy link
Contributor

TotoGaz commented Apr 3, 2023

the whole approach of parsing error messages and attempting to insert text in the middle logically "backwards" and extremely brittle.

You're absolutely correct. I think we would better keep the initial information of the exception in a more meaningful way (not in a raw string). But I'd expect this to be a significantly larger amount of work... And maybe we would even have to deal with the python bindings and I'm totally unaware on how it works.

If someone changes the message format in LvArray logging macro and replaces 5 stars with 4 or something, this functionality might be silently broken.

Most likely not, since this would result in broken unit tests.

Not to stand in the way of merging this as is (it is an improvement for end users), but we should consider an overall change to how throwing macros work: instead of adding all the extra formatting (rank number, stars, etc.) at the source of throw, we should throw just the succinct error message and add all the other stuff at the end (when the message gets printed). This will allow us to easily "stack" multiple contexts (messages) line-by-line as the exception propagates up the call stack, without having to parse the previous text. We could even preserve the original exception object and wrap it in the new one that gets re-thrown (e.g. nested_exception).

This surely makes sense, but...

... if anyone has free cycles to do it.

... this is the eternal issue.

MelReyCG added 3 commits April 5, 2023 17:25
- used stringutilities::cstrlen where it could be
- made InsertExMsg a free function
@MelReyCG MelReyCG requested a review from TotoGaz April 11, 2023 14:24
@@ -334,7 +349,12 @@ class Group
T const & getGroup( KEY const & key ) const
{
Group const * const child = m_subGroups[ key ];
GEOSX_THROW_IF( child == nullptr, "Group " << getPath() << " doesn't have a child " << key, std::domain_error );
if( child == nullptr )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why did you convert GEOSX_THROW_IF into GEOSX_THROW here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to change them back, I previously used std::vector to store the names.

@MelReyCG MelReyCG requested a review from TotoGaz April 14, 2023 07:21
@TotoGaz TotoGaz merged commit f72a0da into develop Apr 25, 2023
@TotoGaz TotoGaz deleted the bugfix/rey/2320-xml-errors-feedback branch April 25, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EPIC] Improve feedback to user on XML errors
4 participants