community: init signature revision for Cassandra LLM cache classes + small maintenance #17765
Conversation
…/warnings, better docstrings
dosubot
bot
added
size:L
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@baskaryan Hello, I'd like your opinion on this one. You may remember we already briefly discussed about the init signature change (several months back). |
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| table_name: str = CASSANDRA_CACHE_DEFAULT_TABLE_NAME, |
There was a problem hiding this comment.
this is a breaking change, would prefer to avoid it
There was a problem hiding this comment.
This is, in a sense, the point of this PR. Using the cassio.init() construct to set a globally-available database connection would enable a much easier constructor pattern where one can omit the session/keyspace arguments altogether when creating table-based LangChain objects.
I am aware this is a breaking change, on the other hand it would make it possible to use this pattern like CassandraCache("table") instead of the more unwieldy CassandraCache(None, None, "table"), something that would really be an advantage in the long run.
Would you have a suggestion on how to best make this transition happen by respecting legacy usage as much as possible?
| *, | ||
| session: Optional[CassandraSession] = None, | ||
| keyspace: Optional[str] = None, | ||
| distance_metric: Optional[str] = None, | ||
| similarity_measure: str = CASSANDRA_SEMANTIC_CACHE_DEFAULT_SIMILARITY, |
There was a problem hiding this comment.
What about marking these whole classes as deprecated and creating brand new ones with the newer init signature, would that be a better way?
(Note our plan would be to align all Cassandra-related classes to this kind of signature eventually)
There was a problem hiding this comment.
👍 This would be an opportunity to rename it to CassandraVectorStore
|
Hello @baskaryan , we have revised our strategy to simplify the init signatures (in view of the global Cassandra connection available through CassIO).
|
|
Update: I reverted the location of the Cassandra cache test file to On the other hand, I have bumped the cassio version in the pyproject.toml of |
dosubot
bot
added
the
lgtm
…small maintenance (#17765) This PR improves on the `CassandraCache` and `CassandraSemanticCache` classes, mainly in the constructor signature, and also introduces several minor improvements around these classes. ### Init signature A (sigh) breaking change is tentatively introduced to the constructor. To me, the advantages outweigh the possible discomfort: the new syntax places the DB-connection objects `session` and `keyspace` later in the param list, so that they can be given a default value. This is what enables the pattern of _not_ specifying them, provided one has previously initialized the Cassandra connection through the versatile utility method `cassio.init(...)`. In this way, a much less unwieldy instantiation can be done, such as `CassandraCache()` and `CassandraSemanticCache(embedding=xyz)`, everything else falling back to defaults. A downside is that, compared to the earlier signature, this might turn out to be breaking for those doing positional instantiation. As a way to mitigate this problem, this PR typechecks its first argument trying to detect the legacy usage. (And to make this point less tricky in the future, most arguments are left to be keyword-only). If this is considered too harsh, I'd like guidance on how to further smoothen this transition. **Our plan is to make the pattern of optional session/keyspace a standard across all Cassandra classes**, so that a repeatable strategy would be ideal. A possibility would be to keep positional arguments for legacy reasons but issue a deprecation warning if any of them is actually used, to later remove them with 0.2 - please advise on this point. ### Other changes - class docstrings: enriched, completely moved to class level, added note on `cassio.init(...)` pattern, added tiny sample usage code. - semantic cache: revised terminology to never mention "distance" (it is in fact a similarity!). Kept the legacy constructor param with a deprecation warning if used. - `llm_caching` notebook: uniform flow with the Cassandra and Astra DB separate cases; better and Cassandra-first description; all imports made explicit and from community where appropriate. - cache integration tests moved to community (incl. the imported tools), env var bugfix for `CASSANDRA_CONTACT_POINTS`. --------- Co-authored-by: Erick Friis <[email protected]>
This PR improves on the
CassandraCacheandCassandraSemanticCacheclasses, mainly in the constructor signature, and also introduces several minor improvements around these classes.Init signature
A (sigh) breaking change is tentatively introduced to the constructor. To me, the advantages outweigh the possible discomfort: the new syntax places the DB-connection objects
sessionandkeyspacelater in the param list, so that they can be given a default value. This is what enables the pattern of not specifying them, provided one has previously initialized the Cassandra connection through the versatile utility methodcassio.init(...).In this way, a much less unwieldy instantiation can be done, such as
CassandraCache()andCassandraSemanticCache(embedding=xyz), everything else falling back to defaults.A downside is that, compared to the earlier signature, this might turn out to be breaking for those doing positional instantiation. As a way to mitigate this problem, this PR typechecks its first argument trying to detect the legacy usage.
(And to make this point less tricky in the future, most arguments are left to be keyword-only).
If this is considered too harsh, I'd like guidance on how to further smoothen this transition. Our plan is to make the pattern of optional session/keyspace a standard across all Cassandra classes, so that a repeatable strategy would be ideal. A possibility would be to keep positional arguments for legacy reasons but issue a deprecation warning if any of them is actually used, to later remove them with 0.2 - please advise on this point.
Other changes
cassio.init(...)pattern, added tiny sample usage code.llm_cachingnotebook: uniform flow with the Cassandra and Astra DB separate cases; better and Cassandra-first description; all imports made explicit and from community where appropriate.CASSANDRA_CONTACT_POINTS.