DM-45978: Add webdav support to ResourcePath.to_fsspec #94

airnandez · 2024-09-26T11:40:52Z

Checklist

ran Jenkins
added a release note for user-visible changes to doc/changes

codecov · 2024-09-26T11:42:07Z

Codecov Report

Attention: Patch coverage is 76.49123% with 67 lines in your changes missing coverage. Please review.

Project coverage is 86.18%. Comparing base (0f26220) to head (729aa57).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
python/lsst/resources/http.py	67.44%	51 Missing and 5 partials ⚠️
tests/test_http.py	90.26%	7 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
- Coverage   87.01%   86.18%   -0.84%     
==========================================
  Files          27       27              
  Lines        4460     4646     +186     
  Branches      905      956      +51     
==========================================
+ Hits         3881     4004     +123     
- Misses        431      487      +56     
- Partials      148      155       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

timj · 2024-09-26T12:08:15Z

python/lsst/resources/http.py

 from urllib.parse import parse_qs

+import aiohttp


I think this will likely need an RFC to formally add it to rubinenv conda environment. I think it comes in through webdav4 at the moment so things will work. Also need to list it in requirements.txt and pyproject.toml (the latter for the http option) -- or else put protections around the import if this is only used with fsspec.

(regardless of import protections we still formally need rubinenv to list it since we want to ensure that this code works in production).

I think aiohttp is already in the environment, since I could find it. In fact, fsspec is built on top of it.

$ source /cvmfs/sw.lsst.eu/almalinux-x86_64/lsst_distrib/w_2024_38/loadLSST.sh $ command -v python /cvmfs/sw.lsst.eu/almalinux-x86_64/lsst_distrib/w_2024_38/conda/envs/lsst-scipipe-9.0.0-exact/bin/python $ python -c "import aiohttp" $ echo $? 0

To be precise, I am not using webdav4. I preferred not to since after 4 weeks I got no response to an issue I submitted to them, so I don't want to depend on that package (see skshetry/webdav4#196)

I agree with Fabio -- I don't think the RFC is needed. We only need aiohttp if we have fsspec, and having fsspec implies aiohttp. The interface Fabio is using requires him to pass an aiohttp client to fsspec, so if fsspec stops using aiohttp internally this is already going to break.

However, I think this import should be part of the try block for fsspec, or lazy-import it in the function below where it is used. We have some services that install this library via pip. fsspec isn't listed as a package dependency so we don't get it by default.

✅ Done: import guarded by same try block used for fsspec

What I'm trying to say is that if we explicitly need aiohttp for this code to work the way we want (ie fsspec working on webdav in the production environment) then we need to make sure that rubin-env comes with aiohttp even if internally fsspec drops that dependency.

Tim -- this code is guaranteed to not work if fsspec drops that dependency in any case. The fsspec constructor signature we are calling is:

HTTPFileSystem(get_client: aiohttp.ClientSession)

So if fsspec stops using aiohttp internally, it's going to break no matter what we do with the conda environment.

dhirving

Looks good to me other than that it breaks to_fsspec for non-webdav HTTP URLs. Should be an easy fix though.

Really good to know that dcache and xrootd support a way to generate pre-signed URLs. That means we can use them as a backend for the Butler server, which might be useful at some point.

dhirving · 2024-09-26T22:10:35Z

python/lsst/resources/http.py

+            ssl_context = ssl.create_default_context()
+            if self._config.ca_bundle is not None:
+                ssl_context.load_verify_locations(capath=self._config.ca_bundle)
+            return aiohttp.ClientSession(connector=aiohttp.TCPConnector(ssl=ssl_context), **kwargs)


I wonder if the TCPConnector should be cached to allow connection pooling, similar to what is done with the requests Session objects.

It might also be good to get the call to load_verify_locations out of the async function, since it does blocking I/O. Probably fast enough it's not a huge deal though.

✅ Done: TCPconnector cached and load_verify_locations called out of the async function.

dhirving · 2024-09-26T22:25:06Z

python/lsst/resources/http.py

+        # Retrieve a signed URL for a duration of 1 hour. We assume the URL
+        # is to be used only for downloading, to avoid the risk the caller
+        # modify its contents inadvertently.
+        url = self.generate_presigned_get_url(expiration_time_seconds=3_600)


This will break to_fsspec for non-webdav HTTP/HTTPS URLs, since fetching a macaroon will fail. Previously it was inheriting the base implementation from ResourcePath.to_fsspec, which just passes the URL unchanged to fsspec.url_to_fs.

Maybe add a conditional at the top delegating to the base class for non-webdav URLs?

✅ Done: conditional added. ResourcePath.to_fsspec called for plain HTTP paths.

dhirving · 2024-09-26T22:27:34Z

python/lsst/resources/http.py

+        except json.JSONDecodeError:
+            raise ValueError(f"could not deserialize response to POST request for URL {self}")
+        except Exception as e:
+            raise e


Since it just re-raises, the behavior would be the same here without the explicit except Exception.

dhirving · 2024-09-26T22:40:27Z

python/lsst/resources/http.py

 from urllib.parse import parse_qs

+import aiohttp


I agree with Fabio -- I don't think the RFC is needed. We only need aiohttp if we have fsspec, and having fsspec implies aiohttp. The interface Fabio is using requires him to pass an aiohttp client to fsspec, so if fsspec stops using aiohttp internally this is already going to break.

However, I think this import should be part of the try block for fsspec, or lazy-import it in the function below where it is used. We have some services that install this library via pip. fsspec isn't listed as a package dependency so we don't get it by default.

dhirving · 2024-09-26T22:43:31Z

python/lsst/resources/http.py

+            case ActivityCaveat.UPLOAD:
+                activity_caveat = "UPLOAD,LIST"
+
+        # Request a macaroon for the requested activities and duration duration


It might be helpful to include a link to some documentation explaining this macaroon stuff...

I eventually found https://www.dcache.org/manuals/workshop-2017-05-29-Umea/000-Final/anupam_macaroons_v02.pdf which helped me figure out what was going on but it took a minute.

✅ Done: pointer to official documentation added.

…in class HttpResourcePathConfig

…o implement to_fsspec

…g a single OPTIONS request

…ow to handle redirection on upload

- cache the aiohttp.TCPConnector to pool network connections by all instances of class HttpResourcePath. - add reference documentation to dCache macaroons. - add specific test for HttpResourcePath.to_fsspec(). - add several type annotations and docstrings.

timj reviewed Sep 26, 2024

View reviewed changes

dhirving approved these changes Sep 26, 2024

View reviewed changes

airnandez added 6 commits October 1, 2024 11:54

Encapsulate all configurable items for instances of HttpResourcePath …

6d67865

…in class HttpResourcePathConfig

Implement URL signing for GET and PUT operations and use signed URL t…

7cc96db

…o implement to_fsspec

Refactor retrieval of server identity and webDAV compliance by sendin…

248ae0c

…g a single OPTIONS request

If remote server is dCache signal the server that this client knows h…

097a887

…ow to handle redirection on upload

Configure mypy to ignore missing imports for aiohttp

d3d3bac

Address reviewer's comments

729aa57

- cache the aiohttp.TCPConnector to pool network connections by all instances of class HttpResourcePath. - add reference documentation to dCache macaroons. - add specific test for HttpResourcePath.to_fsspec(). - add several type annotations and docstrings.

airnandez force-pushed the tickets/DM-45978 branch from 3aee8d8 to 729aa57 Compare October 1, 2024 09:54

airnandez merged commit 754fa0f into main Oct 1, 2024
14 of 17 checks passed

airnandez deleted the tickets/DM-45978 branch October 1, 2024 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-45978: Add webdav support to ResourcePath.to_fsspec #94

DM-45978: Add webdav support to ResourcePath.to_fsspec #94

airnandez commented Sep 26, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

timj Sep 26, 2024

timj Sep 26, 2024

airnandez Sep 26, 2024

airnandez Sep 26, 2024

airnandez Sep 26, 2024

dhirving Sep 26, 2024

airnandez Sep 30, 2024

timj Sep 30, 2024

dhirving Sep 30, 2024

dhirving left a comment

dhirving Sep 26, 2024

dhirving Sep 26, 2024

airnandez Sep 30, 2024

dhirving Sep 26, 2024

airnandez Sep 30, 2024

dhirving Sep 26, 2024

airnandez Sep 30, 2024

dhirving Sep 26, 2024

dhirving Sep 26, 2024

airnandez Sep 30, 2024

DM-45978: Add webdav support to ResourcePath.to_fsspec #94

DM-45978: Add webdav support to ResourcePath.to_fsspec #94

Conversation

airnandez commented Sep 26, 2024

Checklist

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhirving left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 26, 2024 •

edited

Loading