search

JCSDA-internal / ioda-converters

Various converters for getting obs data in and out of IODA
9 stars 4 forks source link

BUFR query: Handle long strings from BUFR files #1325

Closed rmclaren closed 10 months ago

rmclaren commented 1 year ago

Description

Adds the ability to process long strings (bigger than 8 bytes) from BUFR files.

Issue(s) addressed

Resolves #1281

Dependencies

List the other PRs that this PR is dependent on:

rmclaren commented 1 year ago

Just FYI, unit tests won't pass until NCEPLIB-bufr is updated. Waiting for PR and new tag from them.

srherbener commented 11 months ago

Just FYI, unit tests won't pass until NCEPLIB-bufr is updated. Waiting for PR and new tag from them.

Looks like there is a v12.0.1 tag in NCEPLIBS-bufr. Does that contain the update that you need for this PR? Thanks!

rmclaren commented 11 months ago

@srherbener Yes, this is the tag we will need to support this PR.

srherbener commented 11 months ago

spack-stack-1.5.0 contains bufr@12.0.0 so PR #1221 will be covered, but unfortunately this PR will not be covered. One way to handle this would be to create a patch release (spack-stack-1.5.1) which provides bufr@12.0.1. I'll discuss this with the spack-stack developers and see what the best way forward is. Thanks for your patience with this.

rmclaren commented 11 months ago

No problem. Thanks for doing this :)

srherbener commented 11 months ago

Regarding the availability of bufr@12.0.1 in spack-stack, the plan is to create a patch release, spack-stack-1.5.1, which will include bufr@12.0.1. We are currently targeting the release of spack-stack-1.5.1 for the end of October.

rmclaren commented 11 months ago

@srherbener Thanks for the update!

PatNichols commented 10 months ago

@rmclaren Could you resolve the conflicts ? I think we are reading to test it in CI!

BenjaminRuston commented 10 months ago

@srherbener letting the CI run one more time but think we are ready to roll this out

will need to build the 1.5.1 stack locally on my Mac do you see any reason not to proceed

srherbener commented 10 months ago

I just learned that the spack-stack-1.5.1 CI containers were accidentally built with bufr@12.0.0 (not the expected bufr@12.0.1). Note the spack-stack-1.5.1 builds on all the HPC systems were correctly built with bufr@12.0.1 so this issue only impacts the CI testing.

Sure enough, the clang CI test shows the following in the log file:

...
-- The following OPTIONAL packages have not been found:
611 |  
612 | * MKL
613 | * cartopy
614 | * bufr (required version >= 12.0.1)
615
...

So the CI testing builds without bufr skipping the bufr2ioda.x build and testing.

See https://github.com/JCSDA/spack-stack/issues/867 for details.

I think you still have the option of merging (since the HPCs all have spack-stack-1.5.1 with bufr@12.0.1), but thought you should know what is happening with the testing so you can make an informed decision about this.

srherbener commented 10 months ago

One downside of merging is that the bufr2ioda build and testing will then be skipped until the containers are fixed. We should probably coordinate with the INFRA team to decide how best to move forward. Sorry about this.

BenjaminRuston commented 10 months ago

not a problem at all @srherbener and thanks for bringing it up is there an estimated date for the update of the containers

srherbener commented 10 months ago

not a problem at all @srherbener and thanks for bringing it up is there an estimated date for the update of the containers

@BenjaminRuston we are waiting for https://github.com/JCSDA/spack-stack/pull/866 to get merged. Then we can merge https://github.com/JCSDA/spack-stack/pull/868. Then we can build new CI containers.

Let's figure out the date for the new containers with @climbfuji. Hopefully it will be relatively soon.

BenjaminRuston commented 10 months ago

thanks @srherbener I've added them to the description of this issue, but could not add as a formal dependency?

anyway appreciate it

srherbener commented 10 months ago

@BenjaminRuston we decided jcsda/spack-stack/pull/866 is merged, and we decided to use the 1.5.1 tag with a manual edit to patch the CI containers (so we know it's just the one change for the burf version). That means that jcsda/spack-stack/pull/868 is no longer a dependency (but still necessary for moving forward on the develop branch with bufr@12.0.1). The CI container rebuild is underway now.

BenjaminRuston commented 10 months ago

@srherbener thanks really wanted this to hold on until all would pass, are we waiting for another container rebuild the bufr@12.0.1, that will presumably fix this

climbfuji commented 10 months ago

The new containers are already in use, they have 12.0.1. At least they should!

srherbener commented 10 months ago

@BenjaminRuston I think what happened is that these changes never got tested in CI until the containers were updated last Friday. This is because the find_package for bufr in this PR requires version 12.0.1, and if not found the configuration continues without building or testing anything that uses the bufr library. I tried to explain this above (https://github.com/JCSDA-internal/ioda-converters/pull/1325#issuecomment-1802878614 https://github.com/JCSDA-internal/ioda-converters/pull/1325#issuecomment-1802885195) but perhaps I wasn't clear enough - sorry if that's the case.