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

[ManagedDB] Rest API based Thin client for ManagedService #2666

Open
wants to merge 101 commits into
base: main
Choose a base branch
from

Conversation

ProgerDav
Copy link
Contributor

@ProgerDav ProgerDav commented Oct 20, 2023

🚀 🚀 Pull Request

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Creating a thin Rest-API client that communicates with the ManagedService.

Main idea, is that most computations shall happen not on client side, and the wrapper should only handle the communication part, with nice and intuitive wrappers, replicating regular VectorStore Interface.

Things to be aware of

Current version excludes the ability of the user to pass any functions / UDFs. Therefore embedding_function and filter parameters will throw NotImplementedError, if passed. filter only supports a dictionary.

Things to worry about

  1. Backwards-compatibility with regular VectorStore API, in case we decide to fully support it.
  2. Testing of the suite here will likely cover only Deeplake related changes, thus there should be not much focus on sending requests to managed service. Ideally, Backend Tests should guarantee the correctness and performance of APIs used here, while this tests could mock most of it. (unless I miss something)

Additional Context

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Attention: 264 lines in your changes are missing coverage. Please review.

Comparison is base (8fff66c) 84.41% compared to head (ddd1065) 82.33%.

Files Patch % Lines
deeplake/core/dataset/dataset.py 19.68% 102 Missing ⚠️
...rstore/dataset_handlers/managed_dataset_handler.py 59.17% 69 Missing ⚠️
deeplake/client/managed/managed_client.py 25.97% 57 Missing ⚠️
...eplake/core/vectorstore/deep_memory/deep_memory.py 28.57% 15 Missing ⚠️
deeplake/core/vectorstore/vector_search/utils.py 70.37% 8 Missing ⚠️
...ectorstore/vector_search/indra/search_algorithm.py 73.33% 4 Missing ⚠️
deeplake/util/exceptions.py 40.00% 3 Missing ⚠️
...ctorstore/dataset_handlers/dataset_handler_base.py 92.00% 2 Missing ⚠️
deeplake/core/vectorstore/deeplake_vectorstore.py 77.77% 2 Missing ⚠️
deeplake/client/utils.py 50.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2666      +/-   ##
==========================================
- Coverage   84.41%   82.33%   -2.09%     
==========================================
  Files         231      234       +3     
  Lines       26437    26906     +469     
==========================================
- Hits        22316    22152     -164     
- Misses       4121     4754     +633     
Flag Coverage Δ
unittests 82.33% <50.18%> (-2.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@sonarcloud
Copy link

sonarcloud bot commented Oct 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

35.6% 35.6% Coverage
2.1% 2.1% Duplication

@ProgerDav ProgerDav marked this pull request as draft November 2, 2023 11:16
@AdkSarsen AdkSarsen marked this pull request as ready for review December 27, 2023 08:59
Copy link

sonarcloud bot commented Feb 10, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
5.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

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.

9 participants