Skip to content

[MRG] Uses gzip when caching in fetch_openml#11830

Merged
jnothman merged 5 commits intoscikit-learn:masterfrom
thomasjpfan:fetch_openml_gzip
Sep 2, 2018
Merged

[MRG] Uses gzip when caching in fetch_openml#11830
jnothman merged 5 commits intoscikit-learn:masterfrom
thomasjpfan:fetch_openml_gzip

Conversation

@thomasjpfan
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan commented Aug 15, 2018

Reference Issues/PRs

Fixes #11822

What does this implement/fix? Explain your changes.

Adds the HTTP header: Accept-encoding: gzip when data_home is not None.

@thomasjpfan thomasjpfan changed the title MRG: Uses gzip when caching [MRG]: Uses gzip when caching Aug 15, 2018
@thomasjpfan thomasjpfan changed the title [MRG]: Uses gzip when caching [MRG] Uses gzip when caching Aug 15, 2018
@rth
Copy link
Copy Markdown
Member

rth commented Aug 16, 2018

Thanks for your PR!

Could you please add "openml" to the title somewhere? Currently it's not really clear from the title or description what this PR is about. (That would help attracting reviewers).

Also tests (and in particular Travis CI) are failing..

@thomasjpfan thomasjpfan changed the title [MRG] Uses gzip when caching [MRG] Uses gzip when caching in openml Aug 16, 2018
@thomasjpfan thomasjpfan changed the title [MRG] Uses gzip when caching in openml [WIP] Uses gzip when caching in openml Aug 16, 2018
@thomasjpfan thomasjpfan changed the title [WIP] Uses gzip when caching in openml [MRG] Uses gzip when caching in openml Aug 16, 2018
@thomasjpfan thomasjpfan changed the title [MRG] Uses gzip when caching in openml [MRG] Uses gzip when caching in fetch_openml Aug 16, 2018
@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Aug 16, 2018

Ah I see, the test mocks have been updated to account for the gzip feature.

jnothman
jnothman approved these changes Aug 30, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that this results in a faster fetch of MNIST when cache=False?

sklearn/datasets/openml.py Outdated
fsrc = urlopen(_OPENML_PREFIX + openml_path)
with open(local_path, 'wb') as fdst:
req.add_header('Accept-encoding', 'gzip')
fsrc = urlopen(req)
Copy link
Copy Markdown
Member

@jnothman jnothman Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably double check that the http response says it is actually gzipped.

Copy link
Copy Markdown
Member Author

@thomasjpfan thomasjpfan Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the responses' s Content-Encoding is not gzip, I see two options to handle this:

  1. Do another request for without gzip.
  2. Raise an exception.

Which do you prefer?

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Aug 30, 2018

With gzip enabled the response body size is 19.8 MB, without gzip enabled, it is 127.9 MB. With the MNIST datasets, there is little to none speed difference when running fetch_openml since it takes time to uncompress the response. The biggest advantage with using gzip is the 84% savings in the response body size.

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Aug 30, 2018 via email

@rth
Copy link
Copy Markdown
Member

rth commented Aug 30, 2018

When the responses' s Content-Encoding is not gzip, I see two options to handle this:
Do another request for without gzip.
Raise an exception.

Can't you allways set Accept-encoding: gzip in the request, then read the Content-Encoding of the response and gzip or not accordingly?

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Aug 31, 2018

@rth I like the idea. I will update this PR according.

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Aug 31, 2018

@jnothman Previously, when cache=False, gzip was not requested which lead to similar fetch_open timings. With the latest commit, ALL requests are sent with the Accept-encoding: gzip header, which requests in faster downloads and fetch_open timings.

rth
rth reviewed Sep 1, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking nice, but tests are failing with,

AttributeError: MockHTTPResponse instance has no attribute 'tell'

on python 2.7

sklearn/datasets/tests/test_openml.py
return MockHTTPResponse(fp, True)
else:
fp = read_fn(path, 'rb')
return MockHTTPResponse(fp, False)
Copy link
Copy Markdown
Member

@rth rth Sep 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is not tested, can we add a test for it?

Copy link
Copy Markdown
Member Author

@thomasjpfan thomasjpfan Sep 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has been updated to test both branches.

sklearn/datasets/tests/test_openml.py Outdated
path_suffix = '.gz'
read_fn = gzip.open

class MockHTTPResponse():
Copy link
Copy Markdown
Member

@rth rth Sep 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MockHTTPResponse(object) to use new-style classes on Py2, not that it matters much..

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Sep 1, 2018

I update this PR to address the API differences of urlopen between Python 2 and 3.

@jnothman jnothman added this to the 0.20 milestone Sep 2, 2018
rth
rth approved these changes Sep 2, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @thomasjpfan !

@rth
Copy link
Copy Markdown
Member

rth commented Sep 2, 2018

@jnothman Do you have any other comments about this? Should we merge it?

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Sep 2, 2018

I tried it out in our slow Australian internet and was very pleased :)

@jnothman jnothman merged commit 83e7375 into scikit-learn:master Sep 2, 2018
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Sep 2, 2018

Thanks yet again for your great work, @thomasjpfan!

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Sep 2, 2018
@thomasjpfan @jnothman
ENH Uses gzip when caching in fetch_openml (scikit-learn#11830)
d04b9bc
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Sep 17, 2018
@thomasjpfan @jnothman
ENH Uses gzip when caching in fetch_openml (scikit-learn#11830)
8454857
@janvanrijn janvanrijn mentioned this pull request Oct 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman approved these changes

+1 more reviewer

@rth rth rth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.20

Development

Successfully merging this pull request may close these issues.

fetch_openml: Use compressed HTTP responses

3 participants

@thomasjpfan @rth @jnothman