fix(indexers): update from_filespace method to patch batch_size bug#205
Conversation
OverviewAfter discussing with different team members, we have decided to go with suggested approach 2, with an additional warning explicitly telling users that their metadata file is outdated.
The warningsWhen using a if metadata.get("batch_size") is None:
if batch_size is not None:
logging.warning(
"Metadata is outdated (pre v1.1.0) and does not contain a batch_size. Using provided batch_size=%d.",
batch_size,
)
else:
logging.warning(
"Metadata is outdated (pre v1.1.0) and does not contain a batch_size. Defaulting to %d.",
_BATCH_SIZE,
)Extra testingI have additionally tested the This mini test project does the following things using
How to Test
|
lukeroantreeONS
left a comment
There was a problem hiding this comment.
Happy to approve 👍
This is a great PR, with good thought put into the different options, and a sensible final decision & implementation.
Tested using a variety of batch_sizes on creation & re-loading, with VDBs created with v1.0.0 and v1.1.0 and the behaviour is as outlined in the PR, and provides a good user experience.
✨ Summary
VectorStore.from_filespace()fails when loading a vector store whosemetadata.jsonhas"batch_size": null(as produced by v1.0.0, which did not persistbatch_size). This happens when thebatch_sizeargument isNonewhich is the default argument.This PR adds a hardcoded fallback option of
batch_size=128to avoid the situation whereVectorStorecreated before v1.1.0 is loaded in and fails due to not persisting any batch_size.Although hardcoding a value is often seen as bad practice, in this case for the edge case where batch_size and metadata["batch_size"] are both None, this may be the cleanest solution. This would mean that a default batch_size would need to be updated in 2 places when maintaining instead of 1.
Other possible solutions would be to:
__init__method into a__init__that just sets up the config for the file and abuildmethod. This would be similar to scikitlearn and some other libraries. This would mean we could use the__init__method when loading from filespace and take the default batch_size. This is quite clean, but a lot of work for such a small issue and goes against our simple one line setup.__init__andfrom_filespacemethods. This means you only have to update the constant in one place. This doesn't appear very elegant to me though.batch_sizeor setting a default at 128 and not reading it from filespace.batch_sizefor legacy stores if bothbatch_sizeand metadata["batch_size"] are None, telling the user their store was created with v1.0.0 and they must passbatch_sizeexplicitly.from_filespacemethod to rewrite the dictionary with a default of 128 if it is None.From all of these I think the most clean ones are either raising the error and letting the user fix it or rewriting the dictionary to avoid the error in the future.
📜 Changes Introduced
✅ Checklist
🔍 How to Test
On V1.1.0 main, using the
DEMO/general_workflow_demo.ipynbas a start we can add the following after creatingmy_vector_store:Confirm this causes an
IndexBuildError.Then change to this branch and repeat the code and confirm it now loads successfully. Please try any other edge cases you think may be possible.