Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lfs: remove obsolete _LFSFileSystem.get_client() method override #301

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

sisp
Copy link
Collaborator

@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 iterative:main Jan 23, 2024
11 of 13 checks passed
@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