ropensci / rnoaa

R interface to many NOAA data APIs
https://docs.ropensci.org/rnoaa
Other
328 stars 84 forks source link

another cran maintainer email about gefs #335

Closed sckott closed 4 years ago

sckott commented 4 years ago

@potterzot email regarding the new version with changes you made:

This is leaving behind directories named /tmp/oc.$$ (where $$ is the pid) on both Fedora Linux and macOS. E.g.

tawny% ls -R oc.70516/ I7falx Pgbqfv lLU0Sz

On macOS these are empty files, but on Linux these say they are from using cookies with libcurl:

Netscape HTTP Cookie File https://curl.haxx.se/docs/http-cookies.html This file was generated by libcurl! Edit at your own risk.

and Googling suggests that it is the responsibility of the caller to clear these up. In any case, under the CRAN policy they should not be created there even if cleaned up later.

Looks like we do need to figure out how to change where those files are written after all. If you think it will take a while, we may need to pull gefs out for a while until figured out, as we need to fix by 2 weeks from now. Do let me know what you think

potterzot commented 4 years ago

@sckott bummer... Looking at this, these actually look slightly different than the other ones I remove after testing, so let me see if I can do something tomorrow. Otherwise unfortunately I agree we have to take it out.

I also want to look and see if there are any other packages that use ncdf4 to see if they can offer some guidance. Thanks!

On Sat, Nov 16, 2019, 08:28 Scott Chamberlain notifications@github.com wrote:

@potterzot https://github.com/potterzot email regarding the new version with changes you made:

This is leaving behind directories named /tmp/oc.$$ (where $$ is the pid) on both Fedora Linux and macOS. E.g.

tawny% ls -R oc.70516/ I7falx Pgbqfv lLU0Sz

On macOS these are empty files, but on Linux these say they are from using cookies with libcurl:

Netscape HTTP Cookie File https://curl.haxx.se/docs/http-cookies.html This file was generated by libcurl! Edit at your own risk.

and Googling suggests that it is the responsibility of the caller to clear these up. In any case, under the CRAN policy they should not be created there even if cleaned up later.

Looks like we do need to figure out how to change where those files are written after all. If you think it will take a while, we may need to pull gefs out for a while until figured out, as we need to fix by 2 weeks from now. Do let me know what you think

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/rnoaa/issues/335?email_source=notifications&email_token=AADUQ3W2LH6ORSWZLWWE3CDQUANTLA5CNFSM4JOF3MP2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HZZRTJA, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADUQ3X72ULLPNJCYUVZPIDQUANTLANCNFSM4JOF3MPQ .

sckott commented 4 years ago

Thanks for looking at this, sounds good

sckott commented 4 years ago

@potterzot Another email

test_check("rnoaa") Loading required package: rnoaa Error in R_nc4_open: NetCDF: file not found ── 1. Failure: gefs_dimension_values errors (@test-gefs.R#180) ──────────────── gefs_dimension_values(dim = "ens1") threw an error with unexpected message. Expected match: "ens1 is not a valid GEFS dimension. Get valid dimensions with 'gefs_dimensions()'." Actual message: "Error in nc_open trying to open file http://thredds.ucar.edu/thredds/dodsC/grib/NCEP/GEFS/Global_1p0deg_Ensemble/members/GEFS_Global_1p0deg_Ensemble_20191117_0000.grib2"

You really do need to check for the existence of the file first (e.g. download it). For all Internet accesses ....

potterzot commented 4 years ago

@sckott Following the suggestion from CRAN, I think this is going to need a substantial rewrite to download the queried file directly and return a data.frame read from a downloaded netcdf file that is a result of the query, rather than establishing a connection and then making the specific query directly. This would avoid the whole creation of temporary files issue.

The package meteoForecast has what I think is a good example of the goal approach. Unfortunately I'm coming up on a few deadlines and probably won't be able to get this done within the CRAN deadline.

I propose we remove the GEFS module for now, and I'll work on redesigning it over the next few weeks, hoping to reintroduce in mid December. I've created a branch on my fork of rnoaa on which I will work on the rewrite, and I'm about to submit a PR for the master branch that removes gefs entirely.

Thanks for liasoning with CRAN on this and my apologies for the continual bother. Hopefully a rewrite can happen sooner than later.

sckott commented 4 years ago

sounds good