cheshire-cat-ai / core

Production ready AI agent framework
https://cheshirecat.ai
GNU General Public License v3.0
2.14k stars 282 forks source link

Added fallback to handle pip errors during plugin installation #823

Closed jacopopalumbo01 closed 1 month ago

jacopopalumbo01 commented 1 month ago

Description

Closes #821 Added a fallback when pip errors happen. All the new packages installed by the plugin are removed, then the plugin is removed with an error message.

Related to issue #821

Type of change

Checklist:

pieroit commented 1 month ago

Thanks @jacopopalumbo01 this is useful!

pieroit commented 1 month ago

Thanks @jacopopalumbo01 we gave a look at your PR. A couple of observations:

jacopopalumbo01 commented 1 month ago

@pieroit I've updated the PR. Now the plugin is deactivated on pip errors. About pip uninstall, the idea was to follow the #821 idea to maintain as clean as possible the installation.

pieroit commented 1 month ago

Awesome! My concern is, if a dev puts into his requirements something like langchain then the installation will break big time. I see a few minor adjustments needed:

(or maybe I'm not seeing something?)

If you are fed up with this PR, let me know and I'll finish! Appreciate a lot what you are doing Thanks @jacopopalumbo01

jacopopalumbo01 commented 1 month ago

@pieroit thank you for the feedback.

About the first point it can't uninstall previously installed packages by construction. It is uninstalling the requirements provided in the temp file generated from filtered_requirements which contains only the requirements that aren't already installed. Just to confirm I tried by modifying the requirements of the plugin "Mood Music" as follows:

feedparser == 6.0.10
json == 1.6.3
bs4 == 4.12.2
requests == 2.31.0
urllib ==2.0.4
/* I added the requirements below */
pip == 24.0
langchain
pytorch

Then, before trying the installation of the packages I logged the value of filtered_requirements, obtaining:

cheshire_cat_core_dev  | [2024-05-21 17:34:00.458] INFO   cat.mad_hatter.plugin.Plugin._install_requirements::270
cheshire_cat_core_dev  | [
cheshire_cat_core_dev  |     "json == 1.6.3\n",
cheshire_cat_core_dev  |     "bs4 == 4.12.2\n",
cheshire_cat_core_dev  |     "urllib ==2.0.4\n",
cheshire_cat_core_dev  |     "pytorch"
cheshire_cat_core_dev  | ]

So, from this point of view, it is guaranteed that the new mechanism doesn't break the container by uninstalling dependencies of the cat or other plugins.

About the second point, the output in console (again using "Mood Music" plugin as test case) is the following:

cheshire_cat_core_dev  | [2024-05-21 17:45:56.549] INFO   cat.mad_hatter.plugin.Plugin._install_requirements::253
cheshire_cat_core_dev  | "Installing requirements for: mood_music_for_cheshire_cat"
cheshire_cat_core_dev  | ERROR: Could not find a version that satisfies the requirement json==1.6.3 (from versions: none)
cheshire_cat_core_dev  | ERROR: No matching distribution found for json==1.6.3
cheshire_cat_core_dev  | [2024-05-21 17:45:57.070] ERROR  cat.mad_hatter.plugin.Plugin._install_requirements::279
cheshire_cat_core_dev  | "Error during installing mood_music_for_cheshire_cat requirements: Command '['pip', 'install', '--no-cache-dir', '-r', '/tmp/tmph45z39z0']' returned non-zero exit status 1."
cheshire_cat_core_dev  | [2024-05-21 17:45:57.074] INFO   cat.mad_hatter.plugin.Plugin._install_requirements::282
cheshire_cat_core_dev  | "Uninstalling requirements for: mood_music_for_cheshire_cat"

So, there is a first error output which is the error generated by pip, followed by a log.error which specifies the plugin id and the pip command which generated the error. Should i provide additional informations?

pieroit commented 1 month ago

Thanks @jacopopalumbo01, my review was superficial sorry for that

Welcome on board!