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.

Sign up for GitHub

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

Jump to bottom

fix(saving without an index present) #508

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(saving without an index present) #508

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9cb19bc
fix(saving without an index present)
silkspace Sep 25, 2023
d0908d6
Merge branch 'master' into fix/saving
silkspace Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions graphistry/text_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,10 +272,16 @@ def search_graph(

def save_search_instance(self, savepath):
from joblib import dump # type: ignore # need to make this onnx or similar

self.build_index()
search = self.search_index
del self.search_index # can't pickle Annoy
try:
self.build_index()
search = self.search_index
del self.search_index # can't pickle Annoy
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a specific exn we can zero in on here for the expected case to OK?

logger.exception(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

include a message

logger.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be import warnings; ... ? i'm not that versed on python, but to enable structured unit testing, safer sw, etc

"Could not build index, saving without it. Run g.build_index() to build it later"
)
search = None
Copy link
Contributor

Choose a reason for hiding this comment

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

why aren't we raising?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we detect the expected-to-fail scenario ahead of time, skip building in that scenario, and still raise for unexpected errors? This seems to catch too many exn cases otherwise and thus is known-wrong..

dump(self, savepath)
self.search_index = search # add it back
logger.info(f"Saved: {savepath}")
Expand All @@ -285,5 +291,9 @@ def load_search_instance(self, savepath):
from joblib import load # type: ignore # need to make this onnx or similar

cls = load(savepath)
cls.build_index()
try:
cls.build_index()
except Exception as e:
Copy link
Contributor

@lmeyerov lmeyerov Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is there a way for us to know whether to build the index or not vs just doing it, e.g., not hasattr(...) or x is None?

current form is unclear how to maintain and handle otherwise

logger.exception(e)
logger.warn("Could not build index, run g.build_index() to build it later")
Copy link
Contributor

Choose a reason for hiding this comment

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

import warnings; ... ?

Copy link
Contributor

Choose a reason for hiding this comment

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

why aren't we raising?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we detect the expected-to-fail scenario ahead of time, skip building in that scenario, and still raise for unexpected errors? This seems to catch too many exn cases otherwise and thus is known-wrong..

return cls
Loading