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

Added fallback to handle pip errors during plugin installation #823

Merged
merged 2 commits into from
May 21, 2024

Conversation

jacopopalumbo01
Copy link
Contributor

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

@pieroit
Copy link
Member

pieroit commented May 16, 2024

Thanks @jacopopalumbo01 this is useful!

@pieroit
Copy link
Member

pieroit commented May 19, 2024

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

  • why uninstall the plugin on exception? One may be in the process of developing it and does not want the folder to be deleted. Deactivation should be enough ;)
  • is the pip uninstall necessary after catching the exception on pip install ?

@jacopopalumbo01
Copy link
Contributor Author

jacopopalumbo01 commented May 20, 2024

@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
Copy link
Member

pieroit commented May 21, 2024

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:

  • do not uninstall the pip package
  • have a clear error message in the terminal indicating what was the plugin and which req breaks it

(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
Copy link
Contributor Author

@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 pieroit merged commit e84c209 into cheshire-cat-ai:develop May 21, 2024
@pieroit
Copy link
Member

pieroit commented May 21, 2024

Thanks @jacopopalumbo01, my review was superficial sorry for that

Welcome on board!

@jacopopalumbo01 jacopopalumbo01 deleted the feature/piperrors branch May 28, 2024 19:17
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.

None yet

2 participants