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(llamaindex): support both new and legacy llama_index versions #422

Merged
merged 8 commits into from
Feb 19, 2024

Conversation

alex-feel
Copy link
Contributor

This commit fixes issues introduced in 5e5bc88 by enhancing the dynamic loading mechanism for llama-index modules to ensure compatibility across both llama-index-core and llama-index-legacy. Modifications include updates across various instrumentor files to correctly reference modules based on the installed llama-index version. These changes address errors encountered during initialization and ensure seamless operation with both versions of llama-index.

  • Dynamically set _instruments in __init__.py based on package version.
  • Adjust MODULE_NAME in instrumentor files to support new and legacy structures.

This commit fixes issues introduced in 5e5bc88 by enhancing the dynamic loading mechanism for `llama-index` modules to ensure compatibility across both `llama-index-core` and `llama-index-legacy`. Modifications include updates across various instrumentor files to correctly reference modules based on the installed `llama-index` version. These changes address errors encountered during initialization and ensure seamless operation with both versions of `llama-index`.

- Dynamically set `_instruments` in `__init__.py` based on package version.
- Adjust `MODULE_NAME` in instrumentor files to support new and legacy structures.
@alex-feel
Copy link
Contributor Author

I apologize for the oversight in the previous commit. It missed some critical aspects of handling the llama-index package versions. I've now made the necessary adjustments to ensure compatibility with both the core and legacy versions. Thank you for your patience and understanding.

This update refines the dynamic loading logic for the llama-index package, ensuring the LlamaIndexInstrumentor class correctly handles scenarios where neither the core nor legacy versions of the package are installed. It enhances error logging for clearer diagnostics, aligning with proper type expectations by returning an empty collection when necessary, thus maintaining type consistency and improving the robustness of the instrumentation setup.
@alex-feel
Copy link
Contributor Author

It looks like everything is ok now.

@nirga
Copy link
Member

nirga commented Feb 13, 2024

Thanks again @alex-feel! I wonder if it's too much to ask of you to write a small test for this, just to make sure it works on the latest version of llamaindex? You can copy-paste the test structure from other instrumentations like OpenAI.

@alex-feel
Copy link
Contributor Author

Thank you for your feedback and the suggestion to add a test. I would be more than happy to take on this task under normal circumstances. However, I'm currently tied up with an urgent project that demands my full attention and cannot divert my focus at this moment.

Regarding the issue addressed in #423, I want to add that the current fix specifically targets and resolves the problem mentioned. I have conducted manual testing on llama_index.core to ensure the fix is effective and have verified that the imports are correct for llama_index.legacy.

@nirga
Copy link
Member

nirga commented Feb 14, 2024

No worries @alex-feel, I'll add them. Tested it on llama-index v0.9.48 and it didn't work so I'll work on fixing that as well.

@alex-feel alex-feel changed the title fix: address dynamic module loading for llama-index compatibility feat: support both new and legacy llama_index versions Feb 14, 2024
@alex-feel
Copy link
Contributor Author

I've updated this branch with an additional commit that reinstates the original changes which my fix addresses. To recap, the original update was designed to enhance the import logic by dynamically supporting both the new (llama_index.core.llms) and legacy (llama_index.legacy.llms.custom) versions of the llama_index package. This approach utilizes conditional imports and adjusts MODULE_NAME dynamically, ensuring compatibility across different versions of the package. This improvement significantly enhances the flexibility and maintainability of our codebase in accommodating the evolving llama_index library.

@nirga nirga changed the title feat: support both new and legacy llama_index versions fix(llamaindex): support both new and legacy llama_index versions Feb 19, 2024
@nirga nirga force-pushed the fix-llama-index-support branch from ddff887 to 9a49a2b Compare February 19, 2024 22:02
@nirga nirga merged commit 9754db8 into traceloop:main Feb 19, 2024
7 checks passed
@alex-feel
Copy link
Contributor Author

👍

@alex-feel alex-feel deleted the fix-llama-index-support branch February 20, 2024 11:34
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