Use CRDS sync instead of informal URL for HST reffile downloads #53
Conversation
|
This PR will NOT be ready for merging until all the tests actually pass. Working on it now... |
ci_watson/hst_helpers.py
Outdated
| @@ -108,6 +116,8 @@ def download_crds(refname, timeout=30, verbose=False): | |||
|
|
|||
| """ | |||
| refdir = None | |||
| fname = refname | |||
| print('Deprecation Warning: `timeout` parameter no longer needed.') | |||
There was a problem hiding this comment.
It is a bit brute force to print this regardless. You could change the signature to take **kwargs and only emit warning if timeout is given. Also better to use warnings.warn(..., DeprecationWarning).
There was a problem hiding this comment.
Good suggestion. Coming right up...
There was a problem hiding this comment.
Changed to using 'warnings.warn' in e00d94c
ci_watson/hst_helpers.py
Outdated
| from ci_watson.artifactory_helpers import check_url, _download | ||
| # need to insure CRDS has been cached locally | ||
| os.environ['CRDS_SERVER_URL'] = CRDS_SERVER_URL | ||
| os.environ['CRDS_PATH']='./' |
There was a problem hiding this comment.
If you ever run the test locally, is this behavior desirable? Won't this download a bunch of stuff into the working dir?
There was a problem hiding this comment.
Actually, that is the point of using the 'crds sync' call below; namely, only those files which are requested get downloaded whether it is local or remote. I ran this on my Windows system at home and only downloaded the one reference file that was requested without getting any other CRDS file. These definitions, though, are required in order to tell CRDS what server to use and that it should only download the files into the local directory (to start with anyway). I could, though, make this definition dependent on whether or not it was already defined by the user. Would that be safer?
There was a problem hiding this comment.
This change was made in b68f309. It was also tested on my desktop Linux workstation at work (freyja) and when 'iref' was defined to point to the local directory, it only copied the desired reference file as requested, while it did not copy anything when 'iref' was pointing to the already available local online cache (as before). It also still works just fine under Windows as well.
There was a problem hiding this comment.
Does hstcal even run on Windows? 👀
There was a problem hiding this comment.
Just don't want to get into the situation where dev runs this locally, downloads a bunch of files into the working dir (which might be source dir), and then accidentally git add all of them into version control.
make this definition dependent on whether or not it was already defined by the user. Would that be safer?
Yes, I think so.
There was a problem hiding this comment.
My tests have been run primarily on Drizzlepac under Windows, not hstcal, as I have not tried to build hstcal C code under Windows. This definition of the URL was made optional under b68f309 and worked as expected only downloading the requested single reference file on my STScI Linux desktop system.
This comment was marked as resolved.
This comment was marked as resolved.
|
Actually, this package is perfectly compatible with Windows, since I developed and tested this change on my home Windows PC. |
|
This change allowed the Drizzlepac pytest 'hap/test_pipeline.py' test to pass as before when run interactively (under Windows) without downloading any extra files. The same test run interactively under Linux also worked as expected. |
This update replaces use of an informal URL and website with calls to CRDS for downloading HST reference files. It is a 'transparent' replacement in that there are no API changes and no changes in behavior from the previous implementation.