Looks OK to me.
The maintainers are reluctant to enforce module-level dependencies that aren't required by every plugin used by their code, so I think we will need to implement a system like https://www.drupal.org/project/ai_agents/issues/3515453 📌 Agents have silent dependencies Active for the AiCKEditor plugins. This will take longer, but will allow dependencies to be controlled by the plugins that need them.
Nevertheless, the way to do it is through a provider, so you will need to raise a ticket that details the changes required to be able to support it: I'd suggest going step by step as any one change could easily break system for every other plugin. The basis of the ciore AI module is that everything is abstracted so that it can be used by any provider: any changes will need to respect this and work regardless of how the AI is ultimately accessed.
I'm leaving this issue as closed unless one of the maintainers want to reopen it, because this doesn't feel like something the current code can support in a single issue. It may well be that the AI family of modules cannot support an AI that works so differently to the other ones, in which case it may be easier to implement a separate contrib module for webGPU.
It looks like the explorers were using incorrect FAPI elements for the file upload that weren't correctly restricting file type uploads: the element should be restricted only to MP3 for compatibility across the widest range of AI Providers.
I've updated all the explorers and added the help text to the descriptions.
As the maintainer has detailed, the core AI module does not do anything directly with an AI and aklways interacts through a provider. AIUI, the appropriate way to support a WebGPU Ai would be through a new provider module that the other AI functionality can interact with, as per https://project.pages.drupalcode.org/ai/developers/writing_an_ai_provider/
As such, I'm going to close this as won't fix.
Code looks fine but we need details of the issue this change corrects, and steps to recreate so we can test properly.
There is a fork but no merge request: see https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → which may assist.
There is no code here to be reviewed, so setting back to Active.
OK, it sounds to me that what we're saying is this:
- It's complicated, because decisions made in the past mean there's no easy way for this module to know if functionCalls are available in a provider;
- Really it should be fixed at the provider level, but we don't control every provider;
- Anything we do do could introduce breaking changes into either the 1.0 or 1.1 versions of this module.
In lieu of a better solution, I think we should really look at 2 as a quick fix: composer has a mechanism for telling people that certain combinations of versions don't work together and its very easy to implement. I've added and done a MR for adding a composer conflict to the Open AI Provider at https://www.drupal.org/project/ai_provider_openai/issues/3519419 🐛 Version of modules can go out of alignment Active - if that's approved, we can start rolling out similar for the other providers the maintainers here control, and leave this ticket open for a discussion about a more generic solution and when it might be best to implement it.
Have added a composer conflict for the 1.0 branch so it cannot be used with the 1.1 versions of AI or AI Agents, which will resolve this issue for the Open AI provider.
mrdalesmith → created an issue.
Failing tests so set back to needs work.
Added as a fgork to allow tests to run, and you have some test fails so this will need further work.
mrdalesmith → made their first commit to this issue’s fork.
Seems OK to me
Might also be worth looking at the HTML structure of that response: it looks to me from the screenshot that the issue occurs outside of the codeblock this MR adds.
Possibly this would make more sense within the ai_logging sub-module: that already has custom AI-related logs, and moderation failures are something that can happen for any provider type (although I believe Open AI is the only one that moderates by default).
LGTM
Not a maintainer and this is just my 2p worth, but this seems like a lot of code to reinvent "log an error message" for a handful of people who need it. The exception throwing has been standardised as the AI module's method of telling the code that something has gone wrong and not to continue processing (which might involve more AI calls and so costs) and I'd be reluctant to introduce something non-standard for one area as that would become expensive to maintain and cause confusion for users.
I'm not sure why standard Drupal logging of the error with more relevant information included won't suit the desired purpose here?
There are multiple test fails in the code so this can't be merged, but I would maybe wait for the maintainers to chip in about the approach before spending any more time working on it?
Tests are failing because of a coding standards breach, so setting back to needs work.
I can only assume you didn't install through composer, or else had some kind of composer error whilst installing, but I cannot recreate thios issue and the required library is listed in the composer.json file for both versions, so I'm going to close this as works as designed.
Looks good to me.
Yeah I'd recommend forcing ECA to use the root account as an initial test: I'm not 100% familiar with ECA but I wouldn't be surprised if it runs its integrations as anonymous in most cases.
Works well, applies cleanly. i think this is a good addition to the module. RTBC.
The current user tokens should be automatically turned into their values from the logged in user as part of the token generation process: the most obvious thing to check is that there is a currently logged in user at the point that the AI request is being sent: if it's being trggered by the anonymous user (such as if you are running it via the queued process rather than directly) there will be no value. The other thing to check is that the user triggering the update has that value set on their account.
this is a duplicate of https://www.drupal.org/project/ai/issues/3516104 ✨ Add text to image support for OpenAI 4o Image Generation Active and that ticket's response still stands, I'm afraid.
Display inline and works nicely. The chatbot is displayed open by default and the minimise button doesn't seem to do anything: this is a change from the default behaviour when the modal display options are used, so I think it needs to work in a similar fashion. At the very least, the collapse button should collapse the chatbot.
Will need the maintainers to feed back on the styling questions, and also decide whether this change needs to be ported to the deprecated Chatbot block as well.
TBH I would advise against this: plugins are the Drupal way of doing this and allowing code to be discovered and used by any module that wants to. That you find the system complicated is more of an issue with Drupal than it is with the AI module, and anything you do to "improve" it will be non-standard and therefore confusing to people who are familiar with Drupal.
I think if you wanted to, a decent compromise would be to have an implementation of the existing plugins that could discover your simplified tools and run them. Otherwise you risk redesigning Drupal from inside contrib and causing unintended issues everywhere.
Hi @bbruno - not seeing any suggestions here: can you create a fork using the guide at https://www.drupal.org/community/contributor-guide → to add your changes to this issue please?
@bbruno feel free: https://www.drupal.org/community/contributor-guide →
Hi @bbruno - you don't need permission: feel free to create a fork through this ticket and add your suggested changes. If you need help with the process you can use https://www.drupal.org/community/contributor-guide →
Is the second screenshot cropped at all? It seems a strange ratio, and the header (which contains the element you'd use to minimise the block when you aren't using it) seems to be cropped out. I'm wondering of what you are actually experiencing here is the issue detailed in https://www.drupal.org/project/ai/issues/3512670 🐛 AI Chatbot interface height is not adjusted in homepage Active - the block is supposed to float above other elements on the page when it's open.
The branch was out of date, and appears to have originally been split from 1.0 but is being tested against 1.1. I can see you've updated it, but I wonder if that's what's causing the seemingly random test fail? I can't test because the patch version of the MR doesn't apply to the current 1.1 dev branch, but I don't want to break anything by attempting the rebase.
The code itself doesn't look to have any major issues, and seems like a good way of handling a very complex idea.
mrdalesmith → made their first commit to this issue’s fork.
The Basic method provides ONLY the selected field's data as {{ context }}. The tokens you are adding in your rubric do not appear to be valid Drupal tokens (they should be prefixed with the entity type, as in [node:field_model] so there is nothing that can turn these into values.
I would say that this is working as expected: the Basic mode is - as the name suggests - a basic method that does not support complex data structures: you SHOULD be using the token method for a set up that is this complicated, and if you have issues with the translations of your tokens that would be the issue you'd need to resolve.
The alternative would be a hook_requirements check in the 1.1 modules to check that any other AI-family modules were also on the 1.1 versions: @Marcus did you have an idea of which way you want to go?
@Marcus - you've put the ticket to needs review but the MR is in draft: is this ready for review? Can you also explain the reasoning behind this being added here rather than in the AI module's ai_validation sub-module? Given the dependency on another module that most sites won't be using, I would have thought this would have made more sense as its own separate contrib module if we're thinking of deprecating the ai_validations sub-module?
I've put a note on the ticket for the maintainer: I'll close this as a dupe
@marcus Can this be ported to 1.1 as well?
I believe this is a duplicate of https://www.drupal.org/project/ai_agents/issues/3499719 🐛 Use Interfaces instead of Class names in AgentHelper::construct Active , which has already been fixed?
Looks good and works without issue for me. RTBC.
OK, I've tested this as best I can, but I think this will need careful review: we are essentially logging in here as a user with elevated permissions but none of the other features of a user account. This could cause security issues if this user gets saved or otherwise sticks around longer than we think it will. At a more basic level, any code that checks $user->isAnonymous() whilst our user is logged in is going to either throw a fatal error or provide an incorrect answer.
Your video doesn't show you clicking "enable" on the Generate with AI section of the settings: it shows you unchecking the box. To recreate, use the latest dev 1.1 release, check the "enable" box but do not enter a prompt in the text area.
mrdalesmith → created an issue.
Change still works, but the new code changes make the code itself a lot neater. I think this is a better version than the original: setting back to RTBC
mrdalesmith → created an issue.
Selecting the tools when adding a new Agent fires an AJAX rebuild, but the "Detailed Tool Usage" section does not appear within the form unless you save the agent and edit it again: I can't see anything within this change that would cause that - is it a separate issue?
I'm not getting a testable response from the agent itself, but I'd assume that's me rather than the code: I think this needs testing by someone more familiar with setting up agents and when a direct response is suitable. The rest of the UI works, if that helps.
Set to "needs work" at Marcus' request so a test can be identified and built into the testing suite.
No, not really: why does it require an entity to be created? I was just suggesting changing the label so the reference widget you'd created appears in the "reference" list when setting up a new field, rather than as a separate field type.
Also, I'm not a maintainer on this project so I wouldn't completely retool your approach based on my suggestions without any feedback from the actual maintainers. My suggestions have no weight here.
Suspect this is outdated with https://www.drupal.org/project/ai/issues/3517883 📌 Module homepage does not list AI Vector Database (VDB) Providers like AI Providers are listed Active and https://www.drupal.org/project/ai/issues/3517884 📌 Documentation page for providers is out of date Active - would you agree @Marcus?
Looks good to me.
@Marcus can you suggest a good test to see if this interferes with anything? Code looks fine but I can't see what would use it when you approach through the UI.
@Marcus is this a duplicate of https://www.drupal.org/project/ai_agents/issues/3517373 📌 Add a plugin manager definition lister tool Active ?
Whilst a Block Content agent is a valid addition, it might be worth considering if it's possible to have a more generic Entity Agent that can handle any agent: at the minute we have a node agent, a taxonomy agent, a deprecated webform agent and now a block agent, and if we continue this way there will be a lot of agents all doing very similar things. If a generic agent is not possible, possibly a new base class or trait so that we don't duplicate some of the underlying methods.
Set to needs work because of Marcus' comments and the failing tests.
Looks good and useful to remove jQuery: RTBC.
The changes in the MR are exactly the same as the changes in https://www.drupal.org/project/ai_agents/issues/3499719 🐛 Use Interfaces instead of Class names in AgentHelper::construct Active so I'm closing this as a duplicate.
@tim corkerton - have you tried using the changes in the MR to see if that resolves the issue for you?
The Providers in the AI module are separate modules - one for each supported AI service like OpenAI, Gemini or whatever. They're not part of the project, and recipes currently don't support choosing optional modules to install so the decision was made to include the two main ones in the recipe and set them up via the form. So no: as far as I can see the simplest solution is to allow the recipe to support optional configuration and have the user able to proceed with setting up the AI module. It's either that or introducing an ability to select a provider module as part of the set up and have Drupal download and configure the chosen one.
Then we'll say RTBC :)
Yep, looks better and works better :)
Translate Text
Default provider: No default
That looks to be the issue: is it possible to set your chosen provider and model in that section?
The "Warning: Undefined array key "#entity"" error is fixed by the patch in the linked ticket, not by uninstalling any modules.
Setting back to needs work: the previous comment confirms it doesn't work if the user has access to multiple text formats, so this is not ready to merge. Also needs to address bringing the solution inside the existing JS, and removing the need for the form alter.
Just to add, the second error in the description is a duplicate of https://www.drupal.org/project/ai/issues/3513409 🐛 Warning: Undefined array key "#entity" in Drupal\ai_translate\Form\AiTranslateForm Active and should be resolved by the patch there.
Still require the information in my previous comment to investigate this further.
Still not working for me, I'm afraid. Steps to recreate:
- Clean site install.
- Enable AI CKEditor with patch above.
- Update Full HTML text format to use AI CKEditor. Ensure Basic HTML does NOT allow use of CK Editor.
- Create content type with WYSIWYG field that uses Basic HTML as default format.
- Create user with permission to use Basic HTML and Full HTML but NOT permission to use AK CKEditor
- Log in as user and create content with new content type.
The code in the form alter runs when the form is first opened and correctly identifies that a WYSIWYG field is in use and that the logged in user does not have the required permission. However, switching the field's text format from the default Basic HTML (no AI) to Full HTML (uses AI) results in the AI button appearing. There are no errors in console or in watchdog.
The form alter also runs now on every form on the site, even those that can never have CK Editor in use: I think this might be better if the access is checked within the existing CK Editor javascript so that it only runs when CK Editor is present.
Can you confirm which of these provider modules you have installed on the site? https://project.pages.drupalcode.org/ai/providers/matris/
I can't confirm your issue without being able to replicate your set-up.
As your screenshot shows, there is a message asking you to do something before you can translate the menu item, including a link to where you need to do it: I also need you to confirm you have done that, because otherwise it is likely the issue is you have misconfigured the module not that the module has a bug.
Sorry I'm losing you: are you saying "Yes I have followed the instructions to set up an AI Provider (can you link to it for us as well?) and have set the AI's modules default settings to say which provider to use for translations"?
It sounds to be like the most robust solution is for v2.0 to abstract what it needs to out of ai_agents to provide the infrastructure to build and run agents, with the separate AI Agents module then housing the "core" agents. The AI Assistant and AI Chatbot modules could then be deprecated in favour of AI Agents introducing a Chatbot agent, probably with an upgrade path between the two to help the early adopters.
The AI module is ballooning and getting very complicated to use and understand: it's trying to do everything without breaking older versions of itself, and provides multiple things that in practice aren't really that different. The Core AI module should really just be an API to send data of the different supported types to an AI and then do something with the reply: the specific implementations like "put the response into a WYSIWYG field" or "show the response to a user" or "create an entity from the response" should be handled by external modules you can install and use if that's the specific way you want to use AI.
That's going to need very careful planning and management though if updates aren't going to brick a lot of sites.
Can you confirm you have followed the installation instructions for the module, particularly the requirement to choose, install and set up an AI Provider to handle the connection to your chosen AI service? And if so, have you visited the AI Settings page to select a default provider and model for translations?
Applies fine and does what it says it will.
The default value functionality however requires you to set up a default when adding the field type to a node type, which isn't the behaviour of others field: default values are usually optional when setting up a content type, with a "Set default value" checkbox. I think users will expect it to work this way, given it is the standard for Drupal field types.
Possibly the field type should just be an additional item in the Reference category rathewr than its own thing when adding a new field - it doesn't do anything different to the other reference field types - but that may be a personal preference.
There seem to be some commits relating to AI Assistant Block access that have leaked into this MR.
Code all looks good an applies cleanly, but as this is such an under-the-hood change I'm finding it hard to reassure myself it has passed testing. Can you maybe suggest a test or two that would confirm it's working as expected?
This looks good and makes sense. Candidate for porting back to 1.0?
OK, well I get this every time I try to enable the 1.1 dev version, so the MR makes a change that resolves the issue for me: I'll leave it to the maintainers to work out if this is required,.
LGTM then :)
I'm not sure that's an answer to my question? I was asking about the status of this ticket currently being "active", which means it isn't ready to be reviewed or merged.
Tests still failing: https://git.drupalcode.org/issue/ai-3515209/-/jobs/4833128 - the $provider_id variable only exists if the $item is not empty, so it is theoretically possible that you then try to put a non-existent variable in the $form_state. The easiest fix is to give the $provider_id a NULL value outside of your if statement.
Couple of suggestions to improve the code.
mrdalesmith → created an issue.
Fixed the errors in the MR and updated the branch. Attempted to test, but cannot find how to trigger these particular tools to confirm they are working.
mrdalesmith → made their first commit to this issue’s fork.
Looks good, works for me. Constraint exists in 10.2 so fine to replace, in 1.0 and 1.1
Looks good and a simple fix that won't break anything: should this issue be in needs review @ipumpkin?
I'm not quite getting from the description and a review of the codebase exactly what changes you're suggesting here: is the chatbackendInterface intended as a potential drop in replacement for the AiAssistantRunner that sits on the block forms? As far as I can see the chatbots are just forms wrapped around the ai_assistant_api's functions so it isn't immediately obvious which bit you think might be possible to make more flexible.