Skip to content
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

Updating NC_DISPATCH_VERSION #2582

Closed
jedwards4b opened this issue Jan 9, 2023 · 25 comments
Closed

Updating NC_DISPATCH_VERSION #2582

jedwards4b opened this issue Jan 9, 2023 · 25 comments
Assignees

Comments

@jedwards4b
Copy link
Contributor

The parallelio library added netcdf integration some time ago with NC_DISPATCH_VERSION=2. I am now trying to update to the most recent NC_DISPATCH_VERSION=5 but I
can't find any documentation related to the changes required, currently when I try to use version 5 I get an error
NetCDF: HDF error err_num = -101
even though the test should not involve any hdf5 file. Can you point me to anything documenting how to update my
user defined format to dispatch version 5?

@DennisHeimbigner
Copy link
Collaborator

I will update the documentation in netcdf-c/docs/dispatch.md to include the text below. Let me know if you need me to add more information.


Appendix A. Changing NC_DISPATCH_VERSION

When new entries are added to the struct NC_Dispatch type
-- located in *include/netcdf_dispatch.h.in -- it is necessary to
do two things.

  1. Bump the NC_DISPATCH_VERSION number
  2. Modify the existing dispatch tables to include the new entries.
    It if often the case that the new entries do not mean anything for
    a given dispatch table. In that case, the new entries may be set to
    some variant of NC_RO_XXX or NC_NOTNC4_XXX NC_NOTNC3_XXX.

Modifying the dispatch version requires two steps:

  1. Modify the version number in netcdf-c/configure.ac, and
  2. Modify the version number in netcdf-c/CMakeLists.txt.

The two should agree in value.

NC_DISPATCH_VERSION Incompatibility

When dynamically adding a dispatch table
-- in nc_def_user_format (see libdispatch/dfile.c) --
the version of the new table is compared with that of the built-in
NC_DISPATCH_VERSION; if they differ, then an error is returned from
that function.

@WardF
Copy link
Member

WardF commented Jan 9, 2023

Looks good Dennis, if you issue that PR against the branch v4.9.1-wellspring.wif I can get it merged in to the v4.9.1 release.

@DennisHeimbigner
Copy link
Collaborator

I will wait a bit to see if jedwards4b
wants more detail.

@jedwards4b
Copy link
Contributor Author

I think that I understand now, thank you.

DennisHeimbigner added a commit to DennisHeimbigner/netcdf-c that referenced this issue Jan 9, 2023
re: Issue Unidata#2582

Update the file netcdf-c/docs/dispatch.md to include
an appendix that discusses how to properly update the
NC_DISPATCH_VERSION for the dispatch table.
@DennisHeimbigner
Copy link
Collaborator

ok, see PR #2583

@jedwards4b
Copy link
Contributor Author

I'm really confused about something. I am calling nc_create with mode=0x40 but when it calls back to my
PIO_NCINT_create function mode=0x1040 which is enabling netcdf4 when I don't want it. Is this a bug or am
I misunderstanding something?

@jedwards4b
Copy link
Contributor Author

This happens in dinfermodel.c NC_infermodel at line 987 (using netcdf-c main).

@jedwards4b
Copy link
Contributor Author

The assignment of model->format at line 766 prevents use of pnetcdf or netcdf classic formats with NC_UDF0 or NC_UDF1.

@DennisHeimbigner
Copy link
Collaborator

Ed Hartnett did the first implementation of the UDF code.
I recall this and I think we discussed this and decided to limit it to netcdf-4 based implementations -- note to self, I really need to make sure these decisions get documented. I think the reason was part of a general push to de-emphasize use of the netcdf-3 format.

In any case, changing this will cause some back compatibility problems, at least with NOAA. Perhaps Ed can weigh in on this.

@jedwards4b
Copy link
Contributor Author

Interesting - @edwardhartnett also implemented the PIO netcdf integration layer and included tests for netcdf_classic and pnetcdf.
Apparently he did not confirm that the file formats written were those requested in the tests. What if we define a
NC_UDF2 which allows me to use these formats without impacting the current behavior?

@DennisHeimbigner
Copy link
Collaborator

That sounds like a good solution. Do you want to do the PR or shall I.
In any case, I hope Ed chimes in; it is possible that he is willing to
change his code to explicitly set the NC_NETCDF4 flag.

@jedwards4b
Copy link
Contributor Author

I'll give it a try, I would like to understand this layer better.

@DennisHeimbigner
Copy link
Collaborator

I looked at the UDF test -- nc_test4/tst_udf.c -- to see what it did.
You can see that it opens the file with just the NC_UDF0 flag (at least
the first time) and does not specify NC_NETCDF4. So the test case appears
to confirm that UDFx => NC_NETCDF4.

@jedwards4b
Copy link
Contributor Author

In the ncint test in PIO we have:

             int cmode[NUM_MODES] = {NC_PIO, NC_PIO|NC_NETCDF4,                                                                                                                                    
                                    NC_PIO|NC_NETCDF4|NC_MPIIO,                                                                                                                                   
                                    NC_PIO|NC_PNETCDF};                                                                                                                                           
            char mode_name[NUM_MODES][NC_MAX_NAME + 1] = {"classic sequential   ",                                                                                                                
                                                          "netCDF-4 sequential  ",                                                                                                                
                                                          "netCDF-4 parallel I/O",                                                                                                                
                                                          "pnetcdf              "};                  

which I think indicates an intent to have these work with classic and pnetcdf.

@jedwards4b
Copy link
Contributor Author

I think that the deprecation warning is not valid for NC_PNETCDF - if you have integrated pnetcdf and want to open a file in that mode you still need to provide the NC_PNETCDF mode flag to the nc_create_par function, correct?

@jedwards4b jedwards4b mentioned this issue Jan 12, 2023
@edwardhartnett
Copy link
Contributor

Guys sorry I don't remember the details of this implementation!

I am happy that you are carrying it forward Jim, I think it has great potential for PIO users!

Unfortunately I am elbows-deep in the NOAA GRIB2 libraries and cannot help out PIO at this time. ;-(

@DennisHeimbigner
Copy link
Collaborator

Ed, one thing you might be able to check:
When nc_open/nc_create open a PIO UDF file, do they
specify the NC_NETCDF4 flag or only NC_UDF0

@jedwards4b
Copy link
Contributor Author

@DennisHeimbigner I can answer that question - it happens here.

@DennisHeimbigner
Copy link
Collaborator

Sorry, I wasn't clear. I am guessing that PIO clients are either:

  1. calling nc_open/nc_create directly or
  2. calling some wrapper that calls nc_open/nc_create.

In either case, I am asking if those clients or that wrapper explicitly specifying
NC_NETCDF4 as part of the mode argument to nc_open/nc_close.
If they are, then that would mean we are safe in removing that "|= NC_NETCDF4"
from ddispatch.c, and we do not need to add UDF2.
If they are NOT explicitly specifying NC_NETCDF4, then the question is:
can Ed modify those clients or wrappers to explicitly specify NC_NETCDF4 as a mode flag.
If he can, then again, we do not need to do UDF2.

@jedwards4b
Copy link
Contributor Author

If we want a NETCDF4 format then NC_NETCDF4 is set in the pio wrapper to nc_open or nc_create.
The problem is that if we do not want a NETCDF4 file and do not set that in the mode then the code in dinfermodel.c sets it which causes an error. If we choose not to add the UDF2 then I need the code in dinfermodel.c to be changed so that UDF0 and UDF1 allow formats other than NETCDF4 to be used. I would be happy to open an alternative PR with that change if you prefer it.

@jedwards4b
Copy link
Contributor Author

Ed implemented this code in PIO but as far as I know it's not being used anywhere except in internal tests. It was done with NC_DISPATCH_VERSION=2 and has not been updated since. I have some time this week and decided to take it on and to try implementing filters.

@DennisHeimbigner
Copy link
Collaborator

Sure, but we can remove/change the NC_NETCDF4 in ddispatch.c if we have reason to believe that
back incompatibility will not be a problem. My working assumption is that PIO is the only current
external use of UDF, so if it already sets NC_NETCDF4, then we can remove it from ddispatch.c
with no consequences. If PIO does not set it, then we need to go the UDF2 route.

@jedwards4b
Copy link
Contributor Author

YES PIO does explicitly set NC_NETCDF4.

@DennisHeimbigner
Copy link
Collaborator

It occurs to me that we actually do not have to do anything.
If the UDF0, say, dispatch code is not netcdf-4 based, it is free
to ignore/suppress the NC_NETCDF4 flag.
The only situation where this would not work is if we have
a dual UDF iimplementation that supports both netcdf-4 and netcdf-3;
a highly unlikely situation.

@WardF WardF added this to the 4.9.1 milestone Jan 12, 2023
@jedwards4b
Copy link
Contributor Author

First this UDF supports both netcdf-4 and netcdf-3 as well as pnetcdf.
Second - are you suggesting that we let netcdf add the NC_NETCDF4 flag internally
and then strip it away again when I receive the dispatch? I don't like that - I don't think I can tell
whether the flag was added by the user call or internally.

@WardF WardF modified the milestones: 4.9.1, 4.9.2 Feb 13, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants