Refactor local client fs operations to new interface #115
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note for reviewers:
I'm not completely sure about the Configuration object I'm using to pass into the specific implementations of the fs clients. Open to suggestions here. |
fryz
commented
Apr 4, 2024
Comment on lines
+226
to
+228
| # TODO(zfry): think about how to do this better | ||
| conf = LocalFSClientConfig(root_dir=args.directory) | ||
| client = LocalBenchClient(conf) |
There was a problem hiding this comment.
this (and the same thing im doing in the tests) are what I'm not sure about
open to ideas/suggestions
fryz
commented
Apr 4, 2024
Comment on lines
+158
to
+162
| def __init__(self, config, use_s3=False): | ||
| if not use_s3: | ||
| self.fs_client = LocalFSClient(config) | ||
| else: | ||
| raise NotImplementedError("S3 Client not implemented yet") |
There was a problem hiding this comment.
probably should think about a better name here than use_s3 - the intention for now is to write an s3 fs client so we can store results in s3, but I think in the future we'll probably also want to add a client for a nosql db (eg: dynamo)
fryz
commented
Apr 5, 2024
| @@ -176,4 +176,4 @@ def user_login( | |||
|
|
|||
| auth_token = login_cookies.get("Authorization") | |||
|
|
|||
| return auth_token | |||
| return auth_token # type: ignore | |||
There was a problem hiding this comment.
needed this to get mypy working - this wasn't related to my change at all.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this MR
The local bench client interweaved the data loading operations with the client code which exposed the Bench data objects to the higher-level business logic.
After this MR
There is a new local filesystem (fs) client which abstracts the data loading operations from the business logic. This allows for us to swap in separate data loading client implementation (eg: an S3 client, maybe in the future a dynamo or some other nosql client).