🇬🇧United Kingdom @justanothermark

LGTM now

LGTM

Looks good to me

MR created to remove remaining references to MinikanbanAgents.

The only change of note is the use of MinikanbanAgentInterface::JOB_NOT_SOLVABLE in AgentRunner. The check before its use had already been replaced with AiAgentInterface::JOB_NOT_SOLVABLE. This has the same value (0) so there will be no change in behaviour and should have been replaced already.

justanothermark → created an issue.

MR created to remove the unused getRetries mehods.

justanothermark → created an issue.

marcus_johansson → credited justanothermark → .

Ready for review

justanothermark → created an issue.

Not sure how to manage issues for backports but I've created a 1.0.x backport MR for this: https://git.drupalcode.org/project/ai/-/merge_requests/533

Updated with the bundle tag but I've left all the parameters. As Marcus suggests, it's better to provide too much info for future implementations than not enough.

$prompt and $instance aren't used here - should we remove them from the signature, or are we expecting extending classes to override and make use of them?

I'm not sure. I can't think of any cases where $prompt could be useful but I can imagine circumstances where $instance might (if there's a specific provider/model for a task and tagging something about the instance could be useful)?

I wonder if bundle would be worth adding?

That makes sense, I'll update the MR shortly

Looks good to me. RTBC.

Created an MR doing what's proposed above so the idea & the code can be reviewed

justanothermark → created an issue.

Looks good to me. RTBC

Looks good to me. RTBC

Looks good to me. RTBC

Makes sense to me. RTBC

Looks like a good description of the issue and a good fix to me. Marking as RTBC.

justanothermark → made their first commit to this issue’s fork.

Would this be better on `Drupal\search_api\Entity\Server` entity save/presave/similar in case the server is created/updated some other way that hasn't created the collection on the Milvus server?

I think the general approach is good but have a few smaller suggestions before this gets merged. Some are definitely just suggestions and optional while others are things that probably need doing.

1. Use HEREDOC style strings for form values rather than lots of   and string concatenation?
2. createFromArray in ToolsFunctionInput and ToolsPropertyInput don't actually create. They should either be made static and create the objects (e.g. DateTimePlus) or be renamed to setFromArray or similar. Also, because they don't create a new object, they don't clear out old values if the key isn't set in the array passed in. If this is intentional then maybe addFromArray would be better but I suspect one of the other two options is the intent.
3. ToolsFunctionOutput::setArgument and ToolsFunctionOutput::setArguments - set* functions normally update a value or have a key to potentially update a value. These always add to the existing array which isn't consistent with the other functions.
4. If a property is required then the check is whether it is NULL. In PHP a required function parameter can be explicitly passed NULL (depending on types). Are we happy with this difference? If so then it should be documented
5. ToolsFunctionInputInterface - make it clearer that the name is a machine name so that translated names aren't passed in
6. The API used for the Weather tool is only free for non-commercial use so probably shouldn't be included in the generic AI module for everyone (or at the very least this should be made clear but I think it should probably be removed).

- Merge AI Logging LLM settings into the AI logging settings
- Allow a setting that turns off AI summarisation of logs
- If the setting is turned off, put a warning on the report page "This log overview page is significantly improved with AI summaries that can be turned on here"

Pushed these changes to a new branch 3493277-extend-logging-capabilities-v2

justanothermark → made their first commit to this issue’s fork.

I've fixed the small things raised on the MR.

For both of the bigger questions I think we need to think about where the line between Assistant and Chatbot/Block should be in general (probably in a follow-up issue rather than hold this up).

I think I slightly lean the other way. The Assistant should be in charge of the error message (and therefore config) unless the throwException option is set to TRUE and then the code using it does have to handle the errors itself and may use the Assistant error message config if it wishes. In which case we should update AiAssistantApiRunner::process() to use the config when throwException is FALSE. But it's not a strongly-held belief and I could be convinced of the opposite so we should probably get some other opinions. I'm not sure it's worth holding the rest of this up for though?

Looks good to me

A quick search for /index.md within the module suggests the following all need updating:

ai_translate/README.md:7
ai_api_explorer/README.md:14
ai_assistant_api/README.md:12
ai_assistant_api/README.md:24
ai_chatbot/README.md:12
ai_content_suggestions/README.md:1
ai_eca/README.md:9
ai_search/README.md:9

@marcus_johansson As discussed, here's an MR. Some notes:

1. This introduces a minor backwards-compatibility issue by changing the type of Exception thrown. As discussed, we think this is probably okay as it would require other code to be doing custom error handling while calling the AiAssistantApiRunner.
2. I have not included an update hook to set any of the new custom messages. This is intentional because telling end users, for example, that an account is being rate limited or had reached a quota limit may be revealing more information than a site administrator would like. Therefore adding custom messages for this should be a manual change.
3. This MR doesn't change whether the Retry button appears. Firstly, the way deepchat handles errors does not allow for extra properties to be added to the error parameter in the error handler so any matching in the JS would have to be on the message itself (which could be done if we pass all error messages to drupalSettings but that feels messy and not very robust). Secondly, the first thing an end user is going to do when they see an error is try the exact same question again regardless of what the error message actually says so we should make it easy for them to do this.

Thoughts and responses welcome :)

The AI module defines a bunch of Exceptions that the provider modules can use to report back errors from the LLM itself in a consistent way. Exactly how and when they're used is up to each individual provider module but the idea is that they use them as consistently as possible so that the AI module can do something useful (in this case, give a relevant error message to the user).

AiQuotaException is not enough credits on the account.

AiSetupFailureException is meant for when the Drupal side (/connection to LLM) isn't working. The only place I can see it used at the moment is when the API key is saved in Drupal but for some reason it couldn't be loaded. So something is wrong with the setup on the Drupal side rather than with the connection, the LLM or the prompt.

@marcus_johansson Added caching as requested. Worth noting that I've done this with a trait to avoid adding a new argument to the service (which would break backwards compatibility and mean we couldn't merge this fix until 2.x). This also means we can reuse the cache bin in other places if needed.

My mistake, I'd fixed that but not pushed the change. Updated now.

Ah yeah, that makes sense @marcus_johansson. I've added an update hook and implemented hook_requirements to provide warnings with links to the documentation if the site is using advanced mode but doesn't have custom prompts enabled.

Created an MR for this.

I also want to suggest creating a follow-up ticket to remove the `$include_pre_prompt` parameter from `AssistantMessageBuilder::buildMessage()`. We already pass the AIAssistant to this parameter so the Message Builder can check this directly to see whether or not to include the pre prompt. In turn, this means we can simplify `AiAssistantApiRunner::process` and keep all references to the prompt files in a single place. However, this would introduce backwards compatibility issues so I haven't included it as part of the MR.

justanothermark → made their first commit to this issue’s fork.

justanothermark → made their first commit to this issue’s fork.

MR created to implement proposed feature

justanothermark → created an issue.

I've created an MR that prevents the issue by replacing the backslashes with pipe characters (and the reverse when accessing the route).

However, I'm not sure this should be the final fix and we should reconsider whether we want to have fully qualified namespaced classes in the URL or if other alternatives might be better.

justanothermark → created an issue.

Updated the reference.

justanothermark → created an issue.

Merge request for initial import/export created

yautja_cetanu → credited justanothermark → .

justanothermark → created an issue.

justanothermark → created an issue.

justanothermark → created an issue.

The initial work and a very barebones summary has been added. Leaving the ticket open for more details to be added.

justanothermark → created an issue.

justanothermark → created an issue.

marcus_johansson → credited justanothermark → .

Marcus_Johansson → credited justanothermark → .

andrewbelcher → credited justanothermark → .

andrewbelcher → credited justanothermark → .

andrewbelcher → credited justanothermark → .

andrewbelcher → credited justanothermark → .

andrewbelcher → credited justanothermark → .

We were also having this problem in the latest version (1.1.1) on D10. In addition, we were also seeing computed breadcrumbs coming through using the internal path instead of the alias so for a page at https://example.com/en/foo/bar we were getting breadcrumbs of:

* "Home": `https://example.com/en`
* "": `https://example.com/en/node`

instead of:

* "Home": `https://example.com/en`
* "Foo": `https://example.com/en/foo`

The attached patch uses the base field definition changes from #3 but switches to use `$url->toString()` instead of `$url->getInternalPath()`. This fixes the breadcrumb computing to give the correct links & titles and also works for translations. Additionally, this should work for sites that use translations based on domains or query strings or something other than `/[langcode]/` as the first part of the path.

A couple of suggestions from reviewing the MR:

1. Should we undo moving the `langcode` field in to the sidebar? The node form doesn't do this so doing it for taxonomy terms makes them inconsistent.
2. I think we should include the `entity.taxonomy_term.content_translation_edit` route as well.

Added patch which:

* Adds a Unit test case for this issue to `\Drupal\Tests\password_policy_length\Unit\PasswordLengthTest::lengthDataProvider()`
* Replaces the use of `strlen()` with `mb_strlen()` in `\Drupal\password_policy_length\Plugin\PasswordConstraint\PasswordLength::validate()`

justanothermark → created an issue.

justanothermark → created an issue.