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

Extension management #463

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Extension management #463

wants to merge 16 commits into from

Conversation

stijn-uva
Copy link
Member

@stijn-uva stijn-uva commented Nov 11, 2024

Adds a worker that can install and uninstall extensions and webtool controls to manipulate it

Todo:

  • Enable and disable extensions - requires changes to ModuleLoader which currently cannot read from the configuration settings database
  • Better frontend UX - currently requires page refresh to reflect whether extensions have been uninstalled, remains possible to interact with extensions after initiating uninstall
  • Configuration management - what happens to extension-related settings when they are installed/uninstalled? Should recurring jobs be deleted?
  • Docker things - does the frontend also need the extensions installed if running in a separate container?

@stijn-uva stijn-uva requested a review from dale-wahl November 13, 2024 11:37
@dale-wahl
Copy link
Member

Weird bug: One cannot update the admin tag to allow managing extensions. It looks like those admin settings are created when creating the database itself. We could add it to a migrate script, but perhaps there is another way.
image

…sion: remove existing jobs; run `uninstall` function if exists in worker
@dale-wahl
Copy link
Member

Added code to delete workers associated with an extension if it is uninstalled (they cannot run afterwards). If we do not do that, then any existing jobs will log this error ad infinitum.

I think we should automatically enable an extension on its installation. And perhaps also queue the restart worker to follow the manage_extension worker. It would be a lot more seamless. Personally, I cannot imagine wanting to install an extension but not enable it, but I may be missing something. Otherwise you need to first enable the extension and then restart (restart is need to run extension installation scripts).

I wanted to add the enabled check in the helpers find_extensions function (enabled is not being added for the /admin/extensions/ route). But then I realized one might want to enable extensions for individual users and not others. But that may not make sense on that page anyway. It would be more interesting to know if they were enabled for anyone. This probably should be done via datasources, but I suppose that could leave processors active from an extension for all users.

Ok, now I can go back to figuring out this data file structure.

@stijn-uva
Copy link
Member Author

Added code to delete workers associated with an extension if it is uninstalled (they cannot run afterwards). If we do not do that, then any existing jobs will log this error ad infinitum.

I think we should automatically enable an extension on its installation. And perhaps also queue the restart worker to follow the manage_extension worker. It would be a lot more seamless. Personally, I cannot imagine wanting to install an extension but not enable it, but I may be missing something. Otherwise you need to first enable the extension and then restart (restart is need to run extension installation scripts).

Yes, this makes sense 👍

I wanted to add the enabled check in the helpers find_extensions function (enabled is not being added for the /admin/extensions/ route). But then I realized one might want to enable extensions for individual users and not others. But that may not make sense on that page anyway. It would be more interesting to know if they were enabled for anyone. This probably should be done via datasources, but I suppose that could leave processors active from an extension for all users.

I don't think we should support toggling extensions for individual users. As you say we can already toggle the data sources an extension may define on an individual level: we could also add toggles for processors, or a processor can define a configuration option that can be toggled per user. But I think we can do that on the level of processors/data sources generally, not specifically for extensions.

@dale-wahl
Copy link
Member

Ok. So, I have moved the extensions to data/extensions. Using data/ as a volume and storing all, well, data there (e.g., config, logs, datasets, etc). Big commit coming with that... but I finally ran into an issue with the extensions themselves which utilize referential imports. from extensions.wikitools.wikipedia_scraper import WikipediaSearch would need to be from data.extensions.wikitools.wikipedia_scraper import WikipediaSearch. I do not really think I have a work around for that, but wanted to check with you.

@stijn-uva
Copy link
Member Author

Ok. So, I have moved the extensions to data/extensions. Using data/ as a volume and storing all, well, data there (e.g., config, logs, datasets, etc). Big commit coming with that... but I finally ran into an issue with the extensions themselves which utilize referential imports. from extensions.wikitools.wikipedia_scraper import WikipediaSearch would need to be from data.extensions.wikitools.wikipedia_scraper import WikipediaSearch. I do not really think I have a work around for that, but wanted to check with you.

Maybe a symbolic link in the root? Or adding the data directory to sys.path?

It's a minor thing, but it would be neat to be able to import extensions from extensions, it just feels right...

@dale-wahl
Copy link
Member

Both good ideas and I definitely agree that importing from extensions feels right. I hit that change and decided it was a good place to stop for the day. 😂 I will test those solutions out and see.

And ANOTHER thing... I tested out the install/uninstall and am pretty pleased. But then I naturally went to check on datasets I had created from now uninstalled datasources. They look fine, but of course they lose access to map_item. Most of our is_compatible checks do not actually look for map_item and instead look for .ndjson. So there are a lot of processors that look run-able but will fail after uninstall. Le sigh.

@dale-wahl
Copy link
Member

symlink works like a charm!

@dale-wahl
Copy link
Member

dale-wahl commented Feb 18, 2025

Ok! Been testing this awhile both new and old builds. Moved extensions to data/extensions with a symlink from extensions (tested on mac/win/linux). Also moved config to data/config and updated the PATH variables to already be in relation to PATH_ROOT (they can still be absolutes if desired) w/ extensions and config hard coded as we occasionally need them without the config manager. This ensure extensions are in a shared Docker volume. The install_extensions function in migrate.py can actually check whether it is installing to the frontend or backend for any installation scripts (which I use since the frontend does not need things like Firefox in the web studies extensions).

Made some fixes to the manage_extensions worker and fixed a few bugs. Installing now triggers a restart which can download new python packages and run installation scripts if necessary for extensions. Uninstalling removes existing jobs related to an extension. Also find_extensions helper includes enabled status.

Remaining:

  • UI fixes per Stijn's comment
  • I made no decision on whether or not to remove extension settings on uninstall; I personally would keep them
  • Uninstalling extensions may have unintended issues such as those I noted in a prior comment from the lack of a map_item method.
  • Auto-enable a newly installed extension (and possibly add datasources? maybe not)

I did not physically move any files (other than the config files) in existing Docker setups though I did change it so by default only one volume is used. I could still do that if we desire but it does not really matter here.

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