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

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

wants to merge 2 commits into from

Conversation

silkspace
Copy link
Contributor

@silkspace silkspace commented Sep 25, 2023

Originally saving was specific to having an in memory index.
Change allows saving when an index has not been built.

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?

del self.search_index # can't pickle Annoy
except Exception as e:
logger.exception(e)
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

search = self.search_index
del self.search_index # can't pickle Annoy
except Exception as e:
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

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

@lmeyerov lmeyerov Sep 25, 2023

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

cls.build_index()
except Exception as e:
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..

@lmeyerov
Copy link
Contributor

ci failed due to spurious pypi bugs, kicked it off again

logger.warn(
"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..

@lmeyerov lmeyerov self-requested a review November 4, 2023 19:54
Copy link
Contributor

@lmeyerov lmeyerov left a comment

Choose a reason for hiding this comment

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

Exn swallowing logic is unsafe, should instead avoid builds in known-bad scenarios, allowing it to still raise in unexpected scenarios

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.

2 participants