JCSDA-internal / ioda-converters

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

Revert "Refactored QueryRunner::collectData" #1379

Closed danholdaway closed 11 months ago

danholdaway commented 11 months ago

Reverts JCSDA-internal/ioda-converters#1221

This does not compile on Discover. I presume because we don't have the version of BUFR lib that's needed (get_irf_f not found). Fences need to be put in place in the CMake in case the user does not have a modern enough version of the dependent library before merging.

We are using the exact version of spack-stack that is written in the documentation.

rmclaren commented 11 months ago

The error get_irf_f would indicate that the NCEPLIB_bufr lib you are using is not the correct one. Must be 12.0.0 for that PR.

danholdaway commented 11 months ago

That's what I figured. But this should be an optional library.

danholdaway commented 11 months ago

Perhaps you meant to change https://github.com/JCSDA-internal/ioda-converters/blob/cab9db43c6c710d2b1c83e7a41924f40e4c7634e/CMakeLists.txt#L90C27-L90C27 to have a minimum version specified?

rmclaren commented 11 months ago

@danholdaway Your suggested change sounds good to me.

danholdaway commented 11 months ago

OK if you can make a PR with the minimum version you think it needed then we can close this and merge that instead.

danholdaway commented 11 months ago

@rmclaren I think you want to make a new PR with just the change you just pushed rather than pushing it to this branch that reverts all the changes. And I think you want that change to be:

find_package( bufr MINIMUM_VERSION 12.0.0 QUIET )
PatNichols commented 11 months ago

@danholdaway @rmclaren Discover should have spack-stack-1.5.0 extremely soon. When that happens everything should be okay. Check to see if this exists: /gpfsm/dswdev/jcsda/spack-stack/spack-stack-1.5.0/envs/unified-env The documentation on loading the new modules is about to be released.

My 2 cents (about what it's worth) would be to do a PR with the smallest change as suggested by @danholdaway Not revert the whole thing. I can do the PR if it's okay.

CoryMartin-NOAA commented 11 months ago

We should hold off on any code changes requiring spack-stack 1.5.0 until after we have funds appropriated for FY24, as it is likely work at both GMAO and EMC would be derailed until folks like Dan or myself can update our workflows to use the new stack. So by that I mean revert this PR for now.

PatNichols commented 11 months ago

@CoryMartin-NOAA Understood. I will revert this.

CoryMartin-NOAA commented 11 months ago

Will approve but need to remove the commit that @rmclaren added in here I think before it actually is a 'revert' PR.

srherbener commented 11 months ago

We should hold off on any code changes requiring spack-stack 1.5.0 until after we have funds appropriated for FY24, as it is likely work at both GMAO and EMC would be derailed until folks like Dan or myself can update our workflows to use the new stack. So by that I mean revert this PR for now.

One thing to note is that spack-stack-1.4.1 (patch release) contains bufr@12.0.0 so that's an option, as long as spack-stack-1.4.1 is acceptable for moving through a potential government shutdown.

CoryMartin-NOAA commented 11 months ago

I just chatted with @nicholasesposito and I think we are okay with knowing that ioda-converters may not be complete until we can update the default GDAS modules to 1.5.0 as long as 1) it builds without BUFR12 and 2) the instructions for using the spack stack modules for the Skylab release are sufficient. So basically, I think it's okay to not merge this and make the other change if that works for @danholdaway

danholdaway commented 11 months ago

I reverted the single commit that ron made to this revert PR so we can go ahead with reverting things. Then Ron can revert this revert and re-add the commit he made to this revert pr. Make sense?

danholdaway commented 11 months ago

Yes @CoryMartin-NOAA that is fine with me. We can merge https://github.com/JCSDA-internal/ioda-converters/pull/1381 and close this.