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

precedence of error codes #334

Closed
wkliao opened this issue Dec 1, 2016 · 10 comments
Closed

precedence of error codes #334

wkliao opened this issue Dec 1, 2016 · 10 comments

Comments

@wkliao
Copy link
Contributor

wkliao commented Dec 1, 2016

The precedence of NetCDF's error code reporting sometimes is not consistent among APIs, in particular, get/put var and attr APIs. (The precedence decides the seriousness of error code that should be reported first when there are more than one error found.) May I suggest the following precedence.
For put att APIs:
NC_EBADID, NC_EPERM, NC_ENOTVAR, NC_EBADNAME, NC_EBADTYPE, NC_ECHAR, NC_EINVAL, NC_ENOTINDEFINE, NC_ERANGE
For get att APIs:
NC_EBADID, NC_ENOTVAR, NC_EBADNAME, NC_ENOTATT, NC_ECHAR, NC_EINVAL, NC_ERANGE
For put/get variable APIs:
NC_EBADID, NC_EPERM, NC_EINDEFINE, NC_ENOTVAR, NC_ECHAR, NC_EINVALCOORDS, NC_EEDGE, NC_ESTRIDE, NC_EINVAL, NC_ERANGE

For a start, I made changes to libsrc4/nc4attr.c based on the above suggestion.
See 4725c14 before #328 and fe9685d after #328

I am not aware of any specification on error report precedence in NetCDF. But if none exists yet, please let me know whether the above suggestion is fine with you.

@edhartnett
Copy link
Contributor

There is no specification of error precedence. Nor even the idea of one, as far as I am aware. But Ward and Dennis may know differently.

Some thoughts:

  • It's asking kind of a lot from the programmers to find all errors of a certain type. The idiom in netCDF programming (and C in general) is that an internal function call can return an error code. So how do we know what error code it will report?
  • Some error codes are a lot easier to check first, like NC_EINVAL. That is often checked at the beginning of function as parameters are passed in. So you are likely to get that first.

So I am not in favor of requiring a precedence of error codes.

@DennisHeimbigner
Copy link
Collaborator

The reported error is determined by the order in which checks are made in the code.
There are sometimes dependencies within the code, but sometimes it is arbitrary.
To be completely true to a precedence, I think we would have to collect all the errors
in a function and then choose the one with the highest precedence. So, we have to
approximate the precedence.
In your put example, EINVAL might be generated at two different points. First when the
arguments are evaluated, and later when more detailed info is available.
I am not in principle opposed to this, but think that is is tricky undertaking.

@wkliao
Copy link
Contributor Author

wkliao commented Nov 13, 2017

This issue was created almost a year ago. I need to recollect my thought.
The precedence I proposed was for NetCDF developers as a consistent way to check errors. For a NetCDF API, there can only be one error code returned, no matter how many errors found internally. My suggestion is to regulate the returned codes based on the degree of error seriousness.

For example, when an invalid varid is passed by users to nc_put_att_int() and at the same time the argument name contains illegal characters, I, as a NetCDF user, prefer to receive NC_ENOTVAR than NC_EBADNAME or NC_EINVAL, because argument varid is ahead of name and NC_ENOTVAR is more informative than NC_EINVAL. The nice thing of following the precedence is the developers can return the function immediately when an error is detected without continuing to check other possible errors. Thus, the implementation of nc_put_att_int() can check argument varid and returns NC_ENOTVAR without continuing to check argument name. Prior to 4.5.0, I saw inconsistent error codes returned: NC_EBADNAME was returned in some cases and NC_ENOTVAR in others. I do not remember exact the cases. That was long ago :)

FYI, the proposed precedence has been implemented in PnetCDF since 1.8.0.
I can see some of my changes for this issue already appear in NetCDF 4.5.0.

@DennisHeimbigner
Copy link
Collaborator

As I said, I am not opposed to this in principle, but I think it is
a lot harder to implement consistently than one might think.

@WardF
Copy link
Member

WardF commented Nov 13, 2017

It would be a good idea maybe, but I don't feel that the current system is necessarily broken. I agree with @DennisHeimbigner that they're tough to be consistent with (possibly), not just with existing code but then maintaining that moving forward. Given the current backlog, plus my own work (I need to do some things besides managing/merging pull requests) I'd say this is a very low priority, if we even proceed with it.

@edhartnett
Copy link
Contributor

@wkliao I have just recently refactored the get/put code. Take a look at PR #1112 and see if it approximates the error precedence you suggest. If not, now's the time to say so, and I can see if I can accommodate. Moving forward, it seems there is no support to maintaining error precedence as a requirement, but the code is unlikely to change much soon, so if we match it now, it should be good for a while.

Then, can you please close this ticket?

@wkliao
Copy link
Contributor Author

wkliao commented Aug 23, 2018

I am writing a test program that can be used for both PnetCDF and NetCDF, will provide it once done.

@wkliao
Copy link
Contributor Author

wkliao commented Aug 29, 2018

A test program is available in URL below.
https://github.com/Parallel-NetCDF/PnetCDF/blob/master/test/testcases/error_precedence.m4

The compiling instruction is given at the beginning of the source code.
I can see NetCDF master branch (with your #1112 merged) failed in a few places.

@edhartnett
Copy link
Contributor

The decision was made not to make error precedence part of the API. So if you tell me specifically what is not passing your test, I can try and make netcdf pass, but can't promise to. Also I don't want to make this test part of netcdf since that would imply that we are making error precedence part of the API.

So essentially the netcdf-c library reserves the right to return any valid error code when a call is made which can have multiple error responses. I will respect the precedence list on a best-effort basis.

This is not just bad attitude on my part. ;-) The netcdf-c library has a dispatch layer, and some errors are checked there. Then the dispatch layer calls some code through a dispatch table, in libhdf5 for HDF5, libhdf4 for HDF5, libsrc for classic, etc.

So we need to check what we can in the dispatch layer, and then leave the rest of the checking to the format-specific code called by the dispatch table. There are 6 dispatch tables currently in use, with more planned. If we can check for errors in the dispatch layer, we ensure consistency between dispatch tables. And we save ourselves the trouble of maintaining 6+ copies of the error checking code.

But since the dispatch code is called first, this imposes some order on the error codes. All the dispatch layer checking is done before any of the checking within the format-specific code.

@wkliao
Copy link
Contributor Author

wkliao commented Aug 29, 2018

I understand NetCDF does not enforce error precedence. FYI. PnetCDF also has its own dispatcher layer and almost all the error testings are done in that layer.

@wkliao wkliao closed this as completed Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants