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

Refine scalar storage #205

Merged
merged 1 commit into from
Apr 14, 2023
Merged

Refine scalar storage #205

merged 1 commit into from
Apr 14, 2023

Conversation

junjiejiangjjj
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #205 (2620612) into dev (3cc350e) will increase coverage by 0.71%.
The diff coverage is 94.61%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #205      +/-   ##
==========================================
+ Coverage   92.17%   92.89%   +0.71%     
==========================================
  Files          42       42              
  Lines        1278     1365      +87     
==========================================
+ Hits         1178     1268      +90     
+ Misses        100       97       -3     
Impacted Files Coverage Δ
gptcache/manager/data_manager.py 86.79% <ø> (ø)
gptcache/manager/eviction.py 85.18% <50.00%> (+12.96%) ⬆️
gptcache/manager/scalar_data/sql_storage.py 96.33% <96.33%> (ø)
gptcache/manager/scalar_data/base.py 77.55% <100.00%> (+3.13%) ⬆️
gptcache/manager/scalar_data/manager.py 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

@mergify mergify bot added the ci-passed label Apr 14, 2023
@@ -56,7 +61,3 @@ def get_old_access(self, count):
@abstractmethod
def get_old_create(self, count):
pass

@abstractmethod
def close(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not delete it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need the interface.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close the session, and it is also possible to do more things when the cache is turned off in the future

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
please be careful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

gptcache/manager/data_manager.py Outdated Show resolved Hide resolved
gptcache/manager/scalar_data/base.py Show resolved Hide resolved
gptcache/manager/scalar_data/base.py Show resolved Hide resolved
gptcache/manager/eviction.py Outdated Show resolved Hide resolved
gptcache/manager/scalar_data/sql_storage.py Outdated Show resolved Hide resolved
gptcache/manager/scalar_data/sql_storage.py Outdated Show resolved Hide resolved
gptcache/manager/scalar_data/sql_storage.py Outdated Show resolved Hide resolved
@@ -199,9 +199,6 @@ def import_data(
def get_scalar_data(self, res_data, **kwargs):
return self.s.get_data_by_id(res_data[1])

def update_access_time(self, res_data, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recover it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

)
else:
id = Column(Integer, primary_key=True, autoincrement=True)
question = Column(String(6000), nullable=False)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe keep the origin size, 1000

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

self._ques, self._answer = get_models(table_name, db_type)
self._engine = create_engine(self._url)
self.Session = sessionmaker(bind=self._engine) # pylint: disable=invalid-name
self._session = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no use, can delete it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

mark_ids = [i[0] for i in mark_ids]
self._scalar_storage.remove_by_state()
mark_ids = self._scalar_storage.get_ids(deleted=True)
self._scalar_storage.clear_deleted_data()
self._vector_base.delete(mark_ids)

def rebuild(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the call of this method in subsequent development?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next week, we will implement the eviction process. For now, just keep the code correct.

self._ques.__table__.create(bind=self._engine, checkfirst=True)
self._answer.__table__.create(bind=self._engine, checkfirst=True)

def _insert(self, data: CacheData, session: "Session"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use Session, not string, which is sqlalchemy.orm.Session

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com>
@SimFG
Copy link
Collaborator

SimFG commented Apr 14, 2023

/lgtm
/approve

@sre-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: junjiejiangjjj, SimFG

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot merged commit 4340cc7 into zilliztech:dev Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants