-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Updates to error handling #468
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #468 +/- ##
==========================================
- Coverage 94.34% 93.02% -1.33%
==========================================
Files 39 40 +1
Lines 3166 3167 +1
==========================================
- Hits 2987 2946 -41
- Misses 179 221 +42 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! I guess one concern is that we are potentially masking system errors in our tests if they were ever to be thrown and happen to have the same numerical value as the micm error values.
And on that note, in MUSICA, I don't see how we will be able to identify exactly which error was thrown since we have overlapping codes. It seems like we would need to know exactly which part of the code threw this so that we know which enum class
to match the error code to. What am I missing?
Yeah, I was starting to work through that in the MUSICA library. One option might be to use a combination of the category name and the
With this to turn
This way on the user side (even in Fortran, and I'm guessing Python), you could use |
I'm taking a look at this today; getting caught up on the conversation. In C, software error codes are typically negative, and system error codes are typically positive, to avoid collisions/confusion. I'm about to review the PR changes so perhaps it's already in place, but in the netCDF project we have an |
I am a fan of having all of the errors in one place as well. It may mean a long file but it also means we don't need to have a specific error class for each header in micm. @mattldawson would you have anything against a single error file? |
For the error handling, would it be better to add the |
If we do, we might be able to leverage the C++ library: https://en.cppreference.com/w/cpp/utility/source_location |
My thinking is that the errors we return to API users should be clear enough for them to act upon without having to look through our source code. (Problems with configuration files, passing improperly sized arrays to API functions, etc.) If we're just interested in bug checking our code at runtime, we can do this with asserts instead of throwing exceptions. What does everyone think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functionality of this looks good, albeit I'm taking a 10,000 foot view. My one thought is that the code would benefit from some comments; while a lot of it is self evident now, it might be less so down the road. "Code needing more in-line comments" is hardly novel feedback, I acknowledge XD. Particularly since it could apply to any number of my own PRs. But since I'm looking at this as an outsider and not a core developer, the function is less immediately obvious to me, and the lack of comments stands out more.
I like that too. The only thing that's a little unclear to me is how the dependencies would work. If the error codes live in MUSICA, would MICM and TUV-x depend on MUSICA? Or if they live in MICM and TUV-x, would MICM and TUV-x depend on each other? |
If each project maintained a list of errors, their own |
From experience, From a user support perspective, I wonder now about a user sending you details around a problem they had. Would the error be enough for you to debug what's going on? Or would |
I like that idea. Would you expect the error header to just define the
and then update, e.g., the Matrix class to use them:
The only thing might be that we couldn't use the same label for specific errors in different categories. (like if INVALID_VECTOR is an error in another class like SparseMatrix) |
I have gone back and forth over time between 'comment everything' and 'try to write self-documenting code', and I don't know where I'm at right now. I'm happy to add more comments though. Was there anything in particular that you noticed should have more documentation? |
Another benefit to the single file for errors might be that if we just use CPP definitions, we can include the same header in the fortran wrapper code and maybe cut down on the passing back and forth of strings between c and fortran or keeping this list in two places. |
In going through the process of making these changes, I think it would be pretty clear what part of the code threw the error (the error categories are specific to one class, or in one case two classes that extend the same base class, and the individual errors are mostly only thrown in one place, or a few places at most). To me what's important is that these errors always indicate a problem that the user should address, not one that indicates a bug in our code. If we also want to make it easier for users to report bugs in our code, what if we added something like an |
Ah, I see. I am thinking about the code bug and I want the error message to be direct and pointed to the specific source code for faster debugging later. But regarding a user error, what you and others propose here makes sense to me, and using an error header file with enumed error code sounds a good idea. Thanks for your clarification. |
I like this idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Many interesting discussions above. I am not very familiar with the system_error
header but we might be able to create a macro to avoid having to create each error category class to the applicable classes? something like,
MICM_ERROR(category, error code, message)
Just a thought, thanks for the work! I think it's quite tricky to design the error handler since we also have to consider MUSICA.
Based on the discussion, I'll go ahead and create a micm error header and add an internal error category that includes the filename and line number in the message. (I'm not sure if we actually have any run-time checks that I can apply this to, but if I can't find one I'll just add a test that throws this error.) I'll request a re-review once that's ready, but let me know if you would prefer not to review this again and I won't include you in the re-review. Thanks for the discussion everyone! |
namespace micm | ||
enum class MicmConfigErrc | ||
{ | ||
Success = MICM_CONFIGURATION_ERROR_CODE_SUCCESS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have a better error system, should we think about removing the success status since it's not an error? Rather than checking the return value throughout the configuration, we can throw an error when we arrive at one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll do that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
include/micm/util/error.hpp
Outdated
#define MICM_ERROR_CATEGORY_CONFIGURATION "MICM Configuration" | ||
#define MICM_CONFIGURATION_ERROR_CODE_SUCCESS 0 | ||
#define MICM_CONFIGURATION_ERROR_CODE_INVALID_KEY 1 | ||
#define MICM_CONFIGURATION_ERROR_CODE_UNKNOWN_KEY 2 | ||
#define MICM_CONFIGURATION_ERROR_CODE_INVALID_FILE_PATH 3 | ||
#define MICM_CONFIGURATION_ERROR_CODE_NO_CONFIG_FILES_FOUND 4 | ||
#define MICM_CONFIGURATION_ERROR_CODE_CAMP_FILES_NOT_FOUND 5 | ||
#define MICM_CONFIGURATION_ERROR_CODE_CAMP_DATA_NOT_FOUND 6 | ||
#define MICM_CONFIGURATION_ERROR_CODE_INVALID_SPECIES 7 | ||
#define MICM_CONFIGURATION_ERROR_CODE_INVALID_MECHANISM 8 | ||
#define MICM_CONFIGURATION_ERROR_CODE_INVALID_TYPE 9 | ||
#define MICM_CONFIGURATION_ERROR_CODE_OBJECT_TYPE_NOT_FOUND 10 | ||
#define MICM_CONFIGURATION_ERROR_CODE_REQUIRED_KEY_NOT_FOUND 11 | ||
#define MICM_CONFIGURATION_ERROR_CODE_CONTAINS_NON_STANDARD_KEY 12 | ||
#define MICM_CONFIGURATION_ERROR_CODE_MUTUALLY_EXCLUSIVE_OPTION 13 | ||
|
||
#define MICM_ERROR_CATEGORY_JIT "MICM JIT" | ||
#define MICM_JIT_ERROR_CODE_INVALID_MATRIX 1 | ||
#define MICM_JIT_ERROR_CODE_MISSING_JIT_FUNCTION 2 | ||
|
||
#define MICM_ERROR_CATEGORY_PROCESS "MICM Process" | ||
#define MICM_PROCESS_ERROR_CODE_TOO_MANY_REACTANTS_FOR_SURFACE_REACTION 1 | ||
|
||
#define MICM_ERROR_CATEGORY_PROCESS_SET "MICM Process Set" | ||
#define MICM_PROCESS_SET_ERROR_CODE_REACTANT_DOES_NOT_EXIST 1 | ||
#define MICM_PROCESS_SET_ERROR_CODE_PRODUCT_DOES_NOT_EXIST 2 | ||
|
||
#define MICM_ERROR_CATEGORY_RATE_CONSTANT "MICM Rate Constant" | ||
#define MICM_RATE_CONSTANT_ERROR_CODE_MISSING_ARGUMENTS_FOR_SURFACE_RATE_CONSTANT 1 | ||
#define MICM_RATE_CONSTANT_ERROR_CODE_MISSING_ARGUMENTS_FOR_USER_DEFINED_RATE_CONSTANT 2 | ||
|
||
#define MICM_ERROR_CATEGORY_SPECIES "MICM Species" | ||
#define MICM_SPECIES_ERROR_CODE_PROPERTY_NOT_FOUND 1 | ||
#define MICM_SPECIES_ERROR_CODE_INVALID_TYPE_FOR_PROPERTY 2 | ||
|
||
#define MICM_ERROR_CATEGORY_MATRIX "MICM Matrix" | ||
#define MICM_MATRIX_ERROR_CODE_ROW_SIZE_MISMATCH 1 | ||
#define MICM_MATRIX_ERROR_CODE_INVALID_VECTOR 2 | ||
#define MICM_MATRIX_ERROR_CODE_ELEMENT_OUT_OF_RANGE 3 | ||
#define MICM_MATRIX_ERROR_CODE_MISSING_BLOCK_INDEX 4 | ||
#define MICM_MATRIX_ERROR_CODE_ZERO_ELEMENT_ACCESS 5 | ||
|
||
#define MICM_ERROR_CATEGORY_INTERNAL "MICM Internal Error" | ||
#define MICM_INTERNAL_ERROR_CODE_GENERAL 1 | ||
#define MICM_INTERNAL_ERROR_CODE_CUDA 2 | ||
#define MICM_INTERNAL_ERROR_CODE_CUBLAS 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the categories, but I still worry that we could end up in a situation (not sure how), that we only have the integer value. What do #defines gain us over static const strings
and one large enum class
(for which we don't manually define the values)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could go either way. There is something about having one big bucket for things that makes me nervous, but I can't actually think of a real example of how this would be a problem in this case.
The one argument for #defines
is that it allows this file to be included in fortran files as well, and might avoid some duplication or difficult passing of strings, but this isn't a super big deal.
I guess, my suggestion would be to keep the categories until we find ourselves in a situation where we only have the integer code for some reason and decide then if we should reorganize. What do you think?
#if 0 | ||
// Copyright (C) 2023-2024 National Center for Atmospheric Research, | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
// This file defines error categories and codes for MICM | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this instead of #pragma once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we include this in a fortran file, any comments would have to be flagged with !
instead of //
. This was just to get the CPP to remove these lines entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about not including copyright for this specific file? I think that makes sense that we don't want to duplicate the error file for fortran but it could also be confusing for MICM being standalone CPP project to have this unusual usage of macro to cater for fortran. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like this. I left other comments but none of them would prevent this from being merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @mattldawson!
Nothing worth holding this up but we might want to expand on some of the error messages if it makes sense and use the std library to get the function name in one place.
The error messages might be duplicating the condition checks above them which isn't ideal but it might make things easier to debug at the user level if that can be exposed and reported back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing those comments @mattldawson!
Reviewing now as well, although I'm sure I'll be the slowest due to relative unfamiliarity; I don't anticipate finding any issues, but I'm trying to digest what I'm looking at instead of skimming it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you for the work! I have one suggestion but it shouldn't prevent this branch from being merged.
#if 0 | ||
// Copyright (C) 2023-2024 National Center for Atmospheric Research, | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
// This file defines error categories and codes for MICM | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about not including copyright for this specific file? I think that makes sense that we don't want to duplicate the error file for fortran but it could also be confusing for MICM being standalone CPP project to have this unusual usage of macro to cater for fortran. What do you think?
I agree about not making things confusing just to cater to fortran, but I think we should try to have the copyright and license on each source code file. What if we revisit this after the fortran wrapper is updated? It could be the case that we don't even need it in fortran and can just use normal comments. |
(I added this to our post-development review issue #211) |
Sounds good, thank you! |
Adds custom error codes to MICM classes and updates existing error handling to use them. The hope is that in the MUSICA library we can catch these errors and turn them into meaningful codes/messages that can be returned through the C API.
closes #466