-
Notifications
You must be signed in to change notification settings - Fork 206
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
base: master
Are you sure you want to change the base?
Conversation
self.build_index() | ||
search = self.search_index | ||
del self.search_index # can't pickle Annoy | ||
except Exception as e: |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import warnings; ...
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
There was a problem hiding this 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
Originally saving was specific to having an in memory index.
Change allows saving when an index has not been built.