dl tutorial files to tmp directory, then move them once successful #1393

gidden · 2017-05-02T11:52:46Z

tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

I'm not sure how to best add tests for this. Also not sure how best to import shutil. I followed the current pattern for os, but am not sure why that pattern exists in this file. I can clean up further if this is deemed useful.

fmaussion · 2017-05-02T17:14:09Z

xarray/tutorial.py

+        if not _os.path.exists(tmpfile):
+            raise ValueError('File could not be downloaded, please try again')
+
+        _shutil.move(tmpfile, localfile)


I thought that the issue wasn't that the file isn't downloaded, but rather that it is incompletely downloaded? Will this new temporary step solve this?

Is there a way to make urlretrieve more robust, with checksums or something?

gidden · 2017-05-02T17:34:06Z

It seems like urlretrieve is coming directly from urllib. It seems in both Python 2 and 3 ContentTooShort error should be thrown and isnt. Perhaps checksums are the right way to go.

…

On Tue, May 2, 2017 at 7:14 PM, Fabien Maussion ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In xarray/tutorial.py <#1393 (comment)>: > url = '/'.join((github_url, 'raw', 'master', fullname)) - _urlretrieve(url, localfile) + _urlretrieve(url, tmpfile) + + if not _os.path.exists(tmpfile): + raise ValueError('File could not be downloaded, please try again') + + _shutil.move(tmpfile, localfile) I thought that the issue wasn't that the file isn't downloaded, but rather that it is incompletely downloaded? Will this new temporary step than solve this? Is there a way to make urlretrieve more robust, with checksums or something? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1393 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVAEVQ4rFwcddY-kEOYnaEaLHsxf_u9ks5r12RkgaJpZM4NOCuy> .

gidden · 2017-05-02T18:07:57Z

Successful locally with pydata/xarray-data#9:

In [3]: xr.tutorial.load_dataset('air_temperature', github_url='https://github.com/gidden/xarray-data', branch='md5')
Out[3]: 
<xarray.Dataset>
Dimensions:  (lat: 25, lon: 53, time: 2920)
Coordinates:
  * lat      (lat) float32 75.0 72.5 70.0 67.5 65.0 62.5 60.0 57.5 55.0 52.5 ...
  * lon      (lon) float32 200.0 202.5 205.0 207.5 210.0 212.5 215.0 217.5 ...
  * time     (time) datetime64[ns] 2013-01-01 2013-01-01T06:00:00 ...
Data variables:
    air      (time, lat, lon) float64 241.2 242.5 243.5 244.0 244.1 243.9 ...
Attributes:
    Conventions:  COARDS
    title:        4x daily NMC reanalysis (1948)
    description:  Data is from NMC initialized reanalysis\n(4x/day).  These a...
    platform:     Model
    references:   http://www.esrl.noaa.gov/psd/data/gridded/data.ncep.reanaly...

shoyer · 2017-05-02T19:32:16Z

Thanks for taking this on @gidden.

Also not sure how best to import shutil. I followed the current pattern for os, but am not sure why that pattern exists in this file.

I think this exists to work around the fact that IPython shows everything that isn't private in auto-complete (ipython/ipykernel#129). But we should probably switch back to normal imports in this module -- this is really more of an IPython issue.

gidden · 2017-05-02T20:43:18Z

Ok, sounds good. `shutil` was no longer needed, but the md5 checksum was implemented as private. So I think all is well.

…

On Tue, May 2, 2017 at 2:32 PM, Stephan Hoyer ***@***.***> wrote: Thanks for taking this on @gidden <https://github.com/gidden>. Also not sure how best to import shutil. I followed the current pattern for os, but am not sure why that pattern exists in this file. I think this exists to work around the fact that IPython shows everything that isn't private in auto-complete (ipython/ipykernel#129 <ipython/ipykernel#129>). But we should probably switch back to normal imports in this module -- this is really more of an IPython issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVAEW5GuD6D_Y4ANjqM5AnK54h5BRZtks5r14TCgaJpZM4NOCuy> .

shoyer

It's kind of ridiculous that we have to implement this logic ourselves, but this looks good to me.

shoyer · 2017-05-02T20:44:06Z

xarray/tutorial.py

@@ -18,9 +20,17 @@
 _default_cache_dir = _os.sep.join(('~', '.xarray_tutorial_data'))


+def _md5(fname):


Call this something more descriptive like check_file_md5_hash?

should it remain private (for ipython completion) or no?

I wouldn't stress about that. Given that we use it in the script in xarray-data, it's fine either way for me.

gidden · 2017-05-03T15:15:15Z

ok, this should be good to go

shoyer · 2017-05-03T16:30:01Z

Thanks @gidden, this look great to me. Could you kindly add a brief note to the "Bug fixes" section of whats-new.rst?

shoyer · 2017-05-03T16:38:02Z

I'd be slightly more comfortable if we had this being unit tested on Travis-CI, but that's somewhat tricky to setup because we don't wan to require network access for the test suite by default. You could do this by adding a flag like --run-network-tests in conftest.py, and then hooking it through in xarray/tests/__init__.py like --run-flaky. I would be awesome if you could take a look at that and set the Travis/Appveyor tests to test the tutorial, but isn't strictly required to merge this.

gidden · 2017-05-03T16:50:17Z

Hey @shoyer, if I have time later I will try to address the unit test issue. If I'm too late, feel free to pull this in the meantime.

closes pydata#1392

…ds on pydata/xarray-data#9

…ag to pytest cli

gidden · 2017-05-21T15:22:07Z

hey @shoyer, @fmaussion. test decorator added with tutorial dataset unit test. i think this is good to go.

shoyer

one small fix, otherwise looks good to me -- thanks!

shoyer · 2017-05-21T15:37:12Z

xarray/tests/test_tutorial.py


    def setUp(self):
        self.testfile = 'tiny'
        self.testfilepath = os.path.expanduser(os.sep.join(
            ('~', '.xarray_tutorial_data', self.testfile)))
        with suppress(OSError):
-            os.remove(self.testfilepath)
+            os.remove('{}.nc'.format(self.testfilepath))
+            os.remove('{}.md5'.format(self.testfilepath))


This should go in its own suppress block

shoyer · 2017-05-21T19:04:26Z

thanks!

fmaussion reviewed May 2, 2017

View reviewed changes

gidden mentioned this pull request May 2, 2017

add script to make md5 sums and md5s pydata/xarray-data#9

Merged

shoyer reviewed May 2, 2017

View reviewed changes

fmaussion mentioned this pull request May 3, 2017

Download and multiprocessing updates OGGM/oggm#226

Merged

fmaussion approved these changes May 3, 2017

View reviewed changes

gidden force-pushed the tutorial-tmp-files branch from 80b6737 to 440b6da Compare May 3, 2017 16:49

gidden added 5 commits May 21, 2017 16:54

dl tutorial files to tmp directory, then move them once successful

b1dabfe

closes pydata#1392

redo attempt at tutorial checking to use md5 checksums instead. depen…

3abf31f

…ds on pydata/xarray-data#9

rm extraneous import

16133b4

update md5 function name

38d8ec1

update whats-new.rst

349099a

gidden force-pushed the tutorial-tmp-files branch from 440b6da to 349099a Compare May 21, 2017 14:55

gidden added 2 commits May 21, 2017 16:57

fix issue link in whats-new

44cb44d

adding tutorial dataset test, adds conditional --run-network-tests fl…

400762c

…ag to pytest cli

shoyer reviewed May 21, 2017

View reviewed changes

one suppress block per file

f1c8886

shoyer merged commit 3737d26 into pydata:master May 21, 2017

andersy005 mentioned this pull request Oct 17, 2018

tutorial.load_dataset() should prevent against partial downloads #1384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dl tutorial files to tmp directory, then move them once successful #1393

dl tutorial files to tmp directory, then move them once successful #1393

gidden commented May 2, 2017 •

edited

Loading

fmaussion May 2, 2017 •

edited

Loading

gidden commented May 2, 2017 via email

gidden commented May 2, 2017

shoyer commented May 2, 2017

gidden commented May 2, 2017 via email

shoyer left a comment

shoyer May 2, 2017

gidden May 2, 2017

shoyer May 2, 2017

gidden commented May 3, 2017

shoyer commented May 3, 2017

shoyer commented May 3, 2017

gidden commented May 3, 2017

gidden commented May 21, 2017

shoyer left a comment

shoyer May 21, 2017

gidden May 21, 2017

shoyer commented May 21, 2017

		@@ -18,9 +20,17 @@
		_default_cache_dir = _os.sep.join(('~', '.xarray_tutorial_data'))


		def _md5(fname):

dl tutorial files to tmp directory, then move them once successful #1393

dl tutorial files to tmp directory, then move them once successful #1393

Conversation

gidden commented May 2, 2017 • edited Loading

fmaussion May 2, 2017 • edited Loading

Choose a reason for hiding this comment

gidden commented May 2, 2017 via email

gidden commented May 2, 2017

shoyer commented May 2, 2017

gidden commented May 2, 2017 via email

shoyer left a comment

Choose a reason for hiding this comment

shoyer May 2, 2017

Choose a reason for hiding this comment

gidden May 2, 2017

Choose a reason for hiding this comment

shoyer May 2, 2017

Choose a reason for hiding this comment

gidden commented May 3, 2017

shoyer commented May 3, 2017

shoyer commented May 3, 2017

gidden commented May 3, 2017

gidden commented May 21, 2017

shoyer left a comment

Choose a reason for hiding this comment

shoyer May 21, 2017

Choose a reason for hiding this comment

gidden May 21, 2017

Choose a reason for hiding this comment

shoyer commented May 21, 2017

gidden commented May 2, 2017 •

edited

Loading

fmaussion May 2, 2017 •

edited

Loading