fix: support minimal llama-index installations #2516
Conversation
dosubot
bot
added
the
size:M
| if llama_index_version_str is None: | ||
| if llama_index_core_version_str is None: | ||
| raise PackageNotFoundError( | ||
| "Missing `llama_index`. " | ||
| "Install with `pip install llama-index` or " | ||
| "`pip install llama-index-core` for a minimal installation." | ||
| ) |
There was a problem hiding this comment.
why not just combine these conditionals with an or?
There was a problem hiding this comment.
I could combine with an and, but then I would need to do an additional conditional to handle the case where llama-index-core is installed but the instrumentation is out of date.
There are four cases I was checking for:
- The user has neither
llama-indexnorllama-index-coreinstalled. - The user has only
llama-index-coreinstalled, but an old version of the instrumentation. - The user has an old version of
llama-indexinstalled and a modern version of the instrumentation. - The user has a modern version of
llama-indexinstalled and an old version of the instrumentation.
| if instrumentation_version < INSTRUMENTATION_MODERN_VERSION: | ||
| raise IncompatibleLibraryVersionError( | ||
| f"llama-index-core v{llama_index_core_version_str} is not compatible with " | ||
| f"openinference-instrumentation-llama-index v{instrumentation_version_str}. " | ||
| "Please upgrade openinference-instrumentation-llama-index to at least 1.0.0 via " | ||
| "`pip install 'openinference-instrumentation-llama-index>=1.0.0'`." | ||
| ) |
There was a problem hiding this comment.
maybe I'm reading the indentation wrong but I feel this is in the wrong place?
There was a problem hiding this comment.
This condition is checking for the case where llama-index-core is installed but llama-index is not installed. In that case, I am checking whether the current version of the instrumentation is compatible.
There was a problem hiding this comment.
I see, yeah the conditionals a a bit confusing. I think it may be good to explain your logic here a bit as it's quite confusing to map the conditionals
| "Please upgrade openinference-instrumentation-llama-index to at least 1.0.0 via " | ||
| "`pip install 'openinference-instrumentation-llama-index>=1.0.0'`." | ||
| ) | ||
| return True |
There was a problem hiding this comment.
Explain this true - e.g. -core existence means that the instrumentation is modern "enough"
There was a problem hiding this comment.
This return statement corresponds to the case where only llama-index-core is installed and a modern version of our instrumentation is installed.
| llama_index_version_str = None | ||
| try: | ||
| llama_index_version_str = version("llama-index") | ||
| except PackageNotFoundError: | ||
| pass | ||
|
|
||
| llama_index_core_version_str = None | ||
| try: | ||
| llama_index_core_version_str = version("llama-index-core") | ||
| except PackageNotFoundError: | ||
| pass |
There was a problem hiding this comment.
| llama_index_version_str = None | |
| try: | |
| llama_index_version_str = version("llama-index") | |
| except PackageNotFoundError: | |
| pass | |
| llama_index_core_version_str = None | |
| try: | |
| llama_index_core_version_str = version("llama-index-core") | |
| except PackageNotFoundError: | |
| pass | |
| llama_index_version_str = None | |
| llama_index_core_version_str = None | |
| try: | |
| llama_index_version_str = version("llama-index") | |
| except PackageNotFoundError: | |
| pass | |
| try: | |
| llama_index_core_version_str = version("llama-index-core") | |
| except PackageNotFoundError: | |
| pass |
alternatively you could just make a function get_packege_version(package_name: str): Optional[str] that does this. I think that would read cleaner.
|
Simplified, hopefully that is more readable @mikeldking |
Adds support for minimal LlamaIndex installations, i.e., when only the
llama-index-coremodule is installed but not the fullllama-indexpackage.resolves #2509