Can we do this without consume the whole stream? https://docs.python.org/3/library/hashlib.html?highlight=hashlib#examples

I don't think so. I could only think of this: to verify the hash we need to read the stream, so we read it and then return a new stream to keep the methods API consistent.

Did you mean consuming the stream in chunks and updating the hashlib.md5 instance with the chunks instead of reading it all in one go? I don't think that would make any difference.

Yes I mean in chunks.

Currently, with this PR the stream will be read twice, once during the the md5 check and again by the caller of _open_openml_url.

Another option is to have _open_openml_url return the byte content such that the callers do not need to call read.

Done the second option, now reads stream once and passes content along

I'm wondering if we should checksum in in chunks. Could you please take a medium sized OpenML dataset (e.g. MNIST https://www.openml.org/d/554 ) and see computing the checksum with the above way is still reasonable with respect to memory usage (see memory_profiler).

It does not make any difference (see test gist)

I guess it makes sense, from the wiki page:

The main algorithm then uses each 512-bit message block in turn to modify the state

It sounds like a state machine that uses fixed memory (states) irrespective of the size of the stream.

The only optimization that can be done here afaik is that if the md5checksum flag is enabled (default), we could download the arff file in chunks and keep updating md5, which would save us some time. It might add some code complexity though (we will have to manage the chunking with urllib), do the maintainers think it is worth it?

Currently the data is traversed twice. First when the stream is read into memory. Second when hashlib.md5 goes through the data.

Would the goal of this optimization make it so we read the data twice?

In your testing do you find that the checksum increases the time it loads the data?

Added the optimization (it now reads the stream only once, and keeps updating the md5 while reading).

In terms of memory, it will not add any significant overhead as I mentioned, since md5 uses a state-machine.

In terms of time / cpu, here is a quick test result for a medium size dataset:

Existing openml (master branch)

$ python -m timeit -c "from sklearn.datasets import fetch_openml; fetch_openml(data_id=554, data_home=None)"
1 loop, best of 5: 12.4 sec per loop

This PR:

$ python -m timeit -c "from sklearn.datasets import fetch_openml; fetch_openml(data_id=554, data_home=None, verify_checksum=False)"
1 loop, best of 5: 12.3 sec per loop

$ python -m timeit -c "from sklearn.datasets import fetch_openml; fetch_openml(data_id=554, data_home=None, verify_checksum=True)"
1 loop, best of 5: 12.6 sec per loop

This is single iteration but it looks like it is not making it significantly slow

What was the time before the optimization? (831d78b)

I do not think this actually is better than before. The way to get a benefit from chunking is to verify the data the same time it is being encoded by the arrf parser.

The chunking this PR is doing still end up reading the data twice. Once, for checking, and another when the data is being parsed by the arrf parser.

Data was already being read twice, once here and once again here.

This PR maintains the number of times the stream is read.

To make this validation occur during decoding the arff would:

1. Be wasteful if done each time decode is called (currently validation is done only when data is downloaded from internet), future decodes are from a local cached file
2. Would involve adding check in the arff decode logic which is in the externals module.
   Would we prefer to modify this?

I would not prefer not changing the arrf module. I suspect the parsing of the arrf file needs the whole thing in memory. I am happy with previous non chucked version.

831d78b would make the stream be read 3 times, once at download (non chunked), then to validate (all at one go) and then to decode (arff).

In my testing the second validation overhead is insignificant in terms of memory or cpu, so I would not worry too much about it. If you also agree then I will push a revert commit

Okay, lets keep the current version.

Not a stream anymore

Currently the data is traversed twice. First when the stream is read into memory. Second when hashlib.md5 goes through the data.

Would the goal of this optimization make it so we read the data twice?

In your testing do you find that the checksum increases the time it loads the data?

I am concerned that every assignment here will create a new bytes since bytes is immutable. Would bytearray and bytearray.extend be better in this case?

With the early exiting above:

We can early exit here:

if expected_md5 is None:
    return fsrc.read()

With the early exiting:

file_md5.update(data)

Leave a comment that sklearn.datasets._openml.urlopen is already mocked by _monkey_patch_webbased_functions.

The removal of the tmpdir is handled by by pytest. (Also this is causing the windows builds to fail)