search

JCSDA-internal / ioda-converters

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

CI testing needs to add pygrib dependency for new GRIB converters #371

Closed srherbener closed 2 years ago

srherbener commented 3 years ago

A new PR under review (#361) will introduce a GRIB file converter. This PR introduces a new dependency, namely the pygrib python module. Currently the CI test container does not have this dependency which results in the build and test of the GRIB converters to be disabled.

mer-a-o commented 3 years ago

@srherbener Do we need to update containers with pygrib or can we install it as an additional step in CodeBuild?

srherbener commented 3 years ago

@mer-a-o EMC said that the usage of the GRIB converters is limited so no need to update the containers. We can do it as an additional step in CodeBuild for now. However, if it's easier/better to update the containers, then let's do that. pygrib is installable via pip if that helps.

jswhit commented 3 years ago

Note that pygrib requires the ECCODES C library. There is a conda package for both pygrib and eccodes.

jswhit commented 3 years ago

assimilation of MRMS radar data is another possible use case for a grib converter.

srherbener commented 3 years ago

@jswhit, thanks for the information. If GRIB becomes more common, then I think we should install pygrib in the containers, but I'm not sure if we are at that threshold yet.

Won't the conda install or pip install take care of installing eccodes as a dependency? I seem to recall pip install pygrib taking care of installing eccodes.

jswhit commented 3 years ago

conda install will install the library, pip install will not.

mer-a-o commented 3 years ago

Thanks @jswhit. Here it is recommended to use conda. But we don't have conda in our containers. I was able to run ECCODES_DIR=path/to/eccodes pip install pygrib after downloading and installing eccodes. I think the steps are a little too complicated to have as part of CodeBuild and it is best to include it in the container. I can still try to add it to CodeBuild as a temporary solution. @srherbener, what do you think?

srherbener commented 3 years ago

I must have used conda install before because I don't remember having to do anything about the eccodes library.

This isn't urgent so I think the best path forward is to install pygrib in the containers and simply wait until that is complete for the CI testing to pick up the new tests. That way we can avoid spending precious time on a temporary solution. Plus we will be set if/when the pygrib demands increases in the future.

srherbener commented 3 years ago

Note in addition to installing pygrib in the containers, you also need to add -DUSE_PYGRIB=1 to the ecbuild command for ioda-bundle to enable the tests of scripts using pygrib.

srherbener commented 2 years ago

We've got eccodes in the containers now, and EMC has merged 2 converters into develop that use pygrib. Because of this, I think we should add pygrib to the CI test containers.

This is not urgent so I will move this to the backlog.

climbfuji commented 2 years ago

That's a "big" one. Needs eccodes, pyproj, ... !

srherbener commented 2 years ago

@CoryMartin-NOAA how important is it to have pygrib in the containers? What priority should we place on this? Thanks!

Currently the CI testing does not check the two converters using pygrib: test_iodaconv_ims_scf and test_iodaconv_afwa_snod.

climbfuji commented 2 years ago

FWIW, I added it to the spack stack version that I am working on. It's working on my Mac.

CoryMartin-NOAA commented 2 years ago

@srherbener I'm not sure it's super important. These were both added by @YoulongXia-NOAA I believe and IMO things that are in GRIB2 aren't really observations, but these are some derived products the land DA team is using as observations. I think it's a niche enough use case that it is not needed.

FYI EMC's EIB group is in talks to finish up a NCEPLIBS GRIB2 Python interface, that would probably be simpler to include in our stacks (we need GRIB2 for any verification or downstream products), so pygrib may end up being replaced by that for our uses.

climbfuji commented 2 years ago

Sounds good, Cory. In case we need pygrib, it can easily be included in the future. But if not, then even better.

srherbener commented 2 years ago

@CoryMartin-NOAA is it okay to close this issue, and submit a new issue in the future if the need comes up. Thanks!

CoryMartin-NOAA commented 2 years ago

@srherbener fine with me

YoulongXia-NOAA commented 2 years ago

@CoryMartin-NOAA when the interface is ready, I would like to test it. Thank you.

srherbener commented 2 years ago

Thanks @CoryMartin-NOAA. I'll close now.