Skip to content

Conversation

@sisp
Copy link
Contributor

@sisp sisp commented Jan 12, 2024

Some more cleanup: I've removed the obsolete _LFSFileSystem.get_client() method override. It's logically identical with its upstream implementation in dvc-http v2.30.2 (which is the last release before some minor code changes)

     async def get_client(self, **kwargs):
+        import aiohttp
         from aiohttp_retry import ExponentialRetry
-        from dvc_http import make_context
+
+        from .retry import ReadOnlyRetryClient
 
         kwargs["retry_options"] = ExponentialRetry(
             attempts=self.SESSION_RETRIES,

and also with its upstream implementation in dvc-http's current state of the main branch:

-    async def get_client(self, **kwargs):
+    async def get_client(
+        self, ssl_verify, read_timeout, connect_timeout, **kwargs
+    ):
+        import aiohttp
         from aiohttp_retry import ExponentialRetry
-        from dvc_http import make_context
+
+        from .retry import ReadOnlyRetryClient
 
         kwargs["retry_options"] = ExponentialRetry(
             attempts=self.SESSION_RETRIES,
@@ -49,19 +53,18 @@ class _LFSFileSystem(HTTPFileSystem):
         # data blobs. We remove the total timeout, and only limit the time
         # that is spent when connecting to the remote server and waiting
         # for new data portions.
-        connect_timeout = kwargs.pop("connect_timeout")
         kwargs["timeout"] = aiohttp.ClientTimeout(
             total=None,
             connect=connect_timeout,
             sock_connect=connect_timeout,
-            sock_read=kwargs.pop("read_timeout"),
+            sock_read=read_timeout,
         )
 
         kwargs["connector"] = aiohttp.TCPConnector(
             # Force cleanup of closed SSL transports.
             # See https://github.com/iterative/dvc/issues/7414
             enable_cleanup_closed=True,
-            ssl=make_context(kwargs.pop("ssl_verify", None)),
+            ssl=make_context(ssl_verify),
         )
 
         return ReadOnlyRetryClient(**kwargs)

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f5df02a) 77.32% compared to head (0d0caf3) 76.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
- Coverage   77.32%   76.53%   -0.79%     
==========================================
  Files          39       39              
  Lines        4978     4969       -9     
  Branches      898      898              
==========================================
- Hits         3849     3803      -46     
- Misses        975     1000      +25     
- Partials      154      166      +12     

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

@pmrowla pmrowla self-assigned this Jan 12, 2024
@pmrowla pmrowla self-requested a review January 12, 2024 09:08
@pmrowla pmrowla merged commit 9016d86 into treeverse:main Jan 23, 2024
@sisp sisp deleted the refactor/lfs-remove-get-client-method branch January 23, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants