Repository: fsspec/filesystem_spec
Pull request #1631 (2024-06-19 17:09:37)

http file system directed to stream by an "Accept-Ranges": "none" response
## Background

`HTTPFileSystem._open` chooses between `HTTPFile` or `HTTPStreamFile`, and only the former allows random access, by using the `"Range"` header in subsequent HTTP GET requests. Servers do not always respond predictably to partial content requests, but they have the option of explicitly discouraging the use of `"Range"` by including `"Accept-Ranges": "none"` in response headers. It's rarely done, but the lack of a `"Accept-Ranges"` response does not (in practice) indicate the lack of support for the `"Range"` header.

## Changes

Previously, `HTTPFileSystem._open` only used `HTTPStreamFile` when NOT block caching (which is not the default) or when the content size could not be determined in advance (i.e. rarely). This PR makes a response header with `"Accept-Range": "none"` into another rare cause for the switch to streaming.

- `HTTPFileSystem._open` ~~now always makes a `self.info` call (even given `size`) 🤨~~ may see a "partial" key in `self.info` results
- `http._file_info` checks for the `"Accept-Ranges"` header in the response
- The return value of `HTTPFileSystem._info` may include `"partial": False`
- Tested with `test_no_range_support`
  - in its first parameterization of `header`, the `ignore_range` value was irrelevant and removed
  - a new parameterization was added with `accept_range` with `conftest` updated to implement

## Alternative

It seems like an alternative approach is to stick with `HTTPFile` but force `_fetch_all` for read. Please comment if that is preferred ... I haven't figured out what does and doesn't use caching.

~~## Draft~~

~~I'm in the process of working with a NASA DAAC that does not provide partial content to test the approach, and will remove the "Draft" status when successful. Reviews in the meantime are very welcome.~~ ~~Ugh, I give up waiting for them. Please review when able!~~

itcarroll: Testing with the DAAC providing `"Accept-Range": "none"` and `earthaccess` jumped through one hoop (getting `HTTPStreamFile` rather than `HTTPFile`, but revealed another. When I get an `HTTPStreamFile` through `earthaccess.open`, there's an error in `fsspec.caching.BaseCache._fetch` because `self.size` is not set. First step to resolving is making a test to cover it, but I'm currently not sure how to trigger this use of caching directloy (i.e. without `earthaccess.open`). Investigating.
martindurant: You seem to have the same situation as https://github.com/fsspec/filesystem_spec/issues/1626 - there are some suggestions in that thread of what might be done.
itcarroll: I think I'm actually running into nsidc/earthaccess#610. If I understand correctly what you say about the cache type being hard coded to "none" (i.e. BaseCache), then there should not have been a call to the cache (and no problem with the lack of `self.size`) if not for the `earthaccess` bug.

If they agree, I will mark this ready for review. Caching on `HTTPStreamFile` would be great, but a separate (harder!) contribution.
itcarroll: Waiting for a review of nsidc/earthaccess#620, and I won't forget to pick this up after.
martindurant: OK thanks :)
itcarroll: @martindurant The downstream issue has resolved as I'd hoped. I've sync'd this branch and ran tests locally. Ready for workflow approval and review. Thanks for your patience!
martindurant: Perfect, thank you.

Patch:
diff --git a/fsspec/implementations/http.py b/fsspec/implementations/http.py
index 7b5a38b..47dfb88 100644
--- a/fsspec/implementations/http.py
+++ b/fsspec/implementations/http.py
@@ -358,9 +358,10 @@ class HTTPFileSystem(AsyncFileSystem):
         kw = self.kwargs.copy()
         kw["asynchronous"] = self.asynchronous
         kw.update(kwargs)
-        size = size or self.info(path, **kwargs)["size"]
+        info = {}
+        size = size or info.update(self.info(path, **kwargs)) or info["size"]
         session = sync(self.loop, self.set_session)
-        if block_size and size:
+        if block_size and size and info.get("partial", True):
             return HTTPFile(
                 self,
                 path,
@@ -520,9 +521,9 @@ class HTTPFileSystem(AsyncFileSystem):
 
 class HTTPFile(AbstractBufferedFile):
     """
-    A file-like object pointing to a remove HTTP(S) resource
+    A file-like object pointing to a remote HTTP(S) resource
 
-    Supports only reading, with read-ahead of a predermined block-size.
+    Supports only reading, with read-ahead of a predetermined block-size.
 
     In the case that the server does not supply the filesize, only reading of
     the complete file in one go is supported.
@@ -835,10 +836,6 @@ async def _file_info(url, session, size_policy="head", **kwargs):
     async with r:
         r.raise_for_status()
 
-        # TODO:
-        #  recognise lack of 'Accept-Ranges',
-        #                 or 'Accept-Ranges': 'none' (not 'bytes')
-        #  to mean streaming only, no random access => return None
         if "Content-Length" in r.headers:
             # Some servers may choose to ignore Accept-Encoding and return
             # compressed content, in which case the returned size is unreliable.
@@ -853,6 +850,11 @@ async def _file_info(url, session, size_policy="head", **kwargs):
         if "Content-Type" in r.headers:
             info["mimetype"] = r.headers["Content-Type"].partition(";")[0]
 
+        if r.headers.get("Accept-Ranges") == "none":
+            # Some servers may explicitly discourage partial content requests, but
+            # the lack of "Accept-Ranges" does not always indicate they would fail
+            info["partial"] = False
+
         info["url"] = str(r.url)
 
         for checksum_field in ["ETag", "Content-MD5", "Digest"]:


Test patch:
diff --git a/fsspec/implementations/tests/test_http.py b/fsspec/implementations/tests/test_http.py
index 81e438a..568284e 100644
--- a/fsspec/implementations/tests/test_http.py
+++ b/fsspec/implementations/tests/test_http.py
@@ -237,9 +237,13 @@ def test_random_access(server, headers):
 @pytest.mark.parametrize(
     "headers",
     [
-        {"ignore_range": "true", "head_ok": "true", "head_give_length": "true"},
+        # HTTPFile seeks, response headers lack size, assumed no range support
+        {"head_ok": "true", "head_give_length": "true"},
+        # HTTPFile seeks, response is not a range
         {"ignore_range": "true", "give_length": "true"},
         {"ignore_range": "true", "give_range": "true"},
+        # HTTPStreamFile does not seek (past 0)
+        {"accept_range": "none", "head_ok": "true", "give_length": "true"},
     ],
 )
 def test_no_range_support(server, headers):
@@ -247,8 +251,8 @@ def test_no_range_support(server, headers):
     url = server + "/index/realfile"
     with h.open(url, "rb") as f:
         # Random access is not possible if the server doesn't respect Range
-        f.seek(5)
         with pytest.raises(ValueError):
+            f.seek(5)
             f.read(10)
 
         # Reading from the beginning should still work
diff --git a/fsspec/tests/conftest.py b/fsspec/tests/conftest.py
index fb1efb0..413ed71 100644
--- a/fsspec/tests/conftest.py
+++ b/fsspec/tests/conftest.py
@@ -135,10 +135,10 @@ class HTTPTestHandler(BaseHTTPRequestHandler):
             self.rfile.readline()
 
     def do_HEAD(self):
+        r_headers = {}
         if "head_not_auth" in self.headers:
-            return self._respond(
-                403, {"Content-Length": 123}, b"not authorized for HEAD request"
-            )
+            r_headers["Content-Length"] = 123
+            return self._respond(403, r_headers, b"not authorized for HEAD request")
         elif "head_ok" not in self.headers:
             return self._respond(405)
 
@@ -148,23 +148,23 @@ class HTTPTestHandler(BaseHTTPRequestHandler):
             return self._respond(404)
 
         if ("give_length" in self.headers) or ("head_give_length" in self.headers):
-            response_headers = {"Content-Length": len(file_data)}
             if "zero_length" in self.headers:
-                response_headers["Content-Length"] = 0
+                r_headers["Content-Length"] = 0
             elif "gzip_encoding" in self.headers:
                 file_data = gzip.compress(file_data)
-                response_headers["Content-Encoding"] = "gzip"
-                response_headers["Content-Length"] = len(file_data)
-
-            self._respond(200, response_headers)
+                r_headers["Content-Encoding"] = "gzip"
+                r_headers["Content-Length"] = len(file_data)
+            else:
+                r_headers["Content-Length"] = len(file_data)
         elif "give_range" in self.headers:
-            self._respond(
-                200, {"Content-Range": f"0-{len(file_data) - 1}/{len(file_data)}"}
-            )
+            r_headers["Content-Range"] = f"0-{len(file_data) - 1}/{len(file_data)}"
         elif "give_etag" in self.headers:
-            self._respond(200, {"ETag": "xxx"})
-        else:
-            self._respond(200)  # OK response, but no useful info
+            r_headers["ETag"] = "xxx"
+
+        if self.headers.get("accept_range") == "none":
+            r_headers["Accept-Ranges"] = "none"
+
+        self._respond(200, r_headers)
 
 
 @contextlib.contextmanager