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.

Sign up for GitHub

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

Jump to bottom

Freelygive/ai recipe #14

Merged
merged 10 commits into from
Aug 29, 2024
Merged

Freelygive/ai recipe #14

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9debe39
Implemented recipe and install wizard for AI in DXPR
ivanboring Aug 21, 2024
dfffa74
Recipe added and configuration form fixed
ivanboring Aug 22, 2024
4973b16
Resolved conflicts
Neslee Aug 22, 2024
4057ba0
Fixed the model selector, provider uninstall and roles in recipes
ivanboring Aug 22, 2024
12c1794
Fixed so it checks if the key is set correctly
ivanboring Aug 22, 2024
f7309eb
Fixed so the new chat_with_image_vision is set as well
ivanboring Aug 23, 2024
ccd029c
Better error handling on the validation functionality
ivanboring Aug 23, 2024
44c5d1f
Longer form fields for long organization keys
ivanboring Aug 23, 2024
1c3d896
Merge branch 'main' into freelygive/ai-recipe
ivanboring Aug 27, 2024
8de0de0
Double dots on error message removed
ivanboring Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 136 additions & 1 deletion installer/src/Form/ConfigureAPIKeysForm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\StringTranslation\TranslationInterface;
use Drupal\dxpr_builder\Service\DxprBuilderJWTDecoder;
use Drupal\key\Entity\Key;
use OpenAI;
use Symfony\Component\DependencyInjection\ContainerInterface;
use WpAi\Anthropic\AnthropicAPI;

/**
* Defines form for entering DXPR Builder product key and Google Cloud Translation API key.
Expand Down Expand Up @@ -68,7 +71,7 @@ class ConfigureAPIKeysForm extends FormBase implements ContainerInjectionInterfa
public function __construct($root, InfoParserInterface $info_parser,
TranslationInterface $translator,
ConfigFactoryInterface $config_factory,
DxprBuilderJWTDecoder $jwtDecoder) {
DxprBuilderJWTDecoder $jwtDecoder,) {
jjroelofs marked this conversation as resolved.
Show resolved Hide resolved
$this->root = $root;
$this->infoParser = $info_parser;
$this->stringTranslation = $translator;
Expand All @@ -86,6 +89,7 @@ public static function create(ContainerInterface $container) {
$container->get('string_translation'),
$container->get('config.factory'),
$container->get('dxpr_builder.jwt_decoder'),
$container->get('module_installer')
);
}

Expand Down Expand Up @@ -119,6 +123,39 @@ public function buildForm(array $form, FormStateInterface $form_state, array &$i
];
// }

$form['ai_provider'] = [
'#type' => 'select',
'#title' => $this->t('Select AI Provider'),
'#description' => $this->t('If you want to enable AI features like ai powered alt text generation, select the AI provider you want to use for AI features and fill in the API Key.'),
'#empty_option' => $this->t('No AI'),
'#options' => [
'openai' => $this->t('OpenAI'),
'anthropic' => $this->t('Anthropic'),
],
];

$form['openai_key'] = [
'#type' => 'password',
'#title' => $this->t('OpenAI API key'),
'#description' => $this->t('Get a key from <a href="https://platform.openai.com/api-keys" target="_blank">platform.openai.com/api-keys</a>.'),
'#states' => [
'visible' => [
':input[name="ai_provider"]' => ['value' => 'openai'],
],
],
];

$form['anthropic_key'] = [
'#type' => 'password',
'#title' => $this->t('Anthropic API key'),
'#description' => $this->t('Get a key from <a href="https://console.anthropic.com/settings/keys" target="_blank">console.anthropic.com/settings/keys</a>.'),
'#states' => [
'visible' => [
':input[name="ai_provider"]' => ['value' => 'anthropic'],
],
],
];

$form['actions'] = [
'continue' => [
'#type' => 'submit',
Expand All @@ -145,6 +182,28 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
if (!empty($google_translation_key)) {
$this->configFactory->getEditable('tmgmt.translator.google')->set('settings.api_key', $google_translation_key)->save();
}

// If the AI provider is set, enable the appropriate modules.
if ($ai_provider = $form_state->getValue('ai_provider')) {
// Setup the key for the provider.
$key_id = $ai_provider . '_key';
$key = Key::create([
'id' => $key_id,
'label' => ucfirst($ai_provider) . ' API Key',
'description' => 'API Key for ' . ucfirst($ai_provider),
'key_type' => 'authentication',
'key_provider' => 'config',
]);
$key->setKeyValue($form_state->getValue($key_id));
$key->save();
// Add the key to the config.
$this->configFactory->getEditable('provider_' . $ai_provider . '.settings')->set('api_key', $key_id)->save();
// Set the default provider.
$this->configFactory->getEditable('ai.settings')->set('default_providers.chat', [
'provider_id' => $ai_provider,
'model_id' => $ai_provider == 'openai' ? 'gpt-4o' : 'claude-3-5-sonnet-20240620',
])->save();
}
jjroelofs marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -164,7 +223,83 @@ public function validateForm(array &$form, FormStateInterface $form_state): void
]));
}
}

// Test the OpenAI key if it is set.
if ($form_state->getValue('ai_provider') === 'openai') {
// It has to be set.
if (empty($form_state->getValue('openai_key'))) {
$form_state->setErrorByName('openai_key', $this->t('OpenAI API key is required, if you want to enable OpenAI.'));
}
// It has to be valid.
elseif ($errorMessage = $this->testOpenAIKey($form_state->getValue('openai_key'))) {
$form_state->setErrorByName('openai_key', $this->t('Your OpenAI API key seems to be invalid with message %message.', [
'%message' => $errorMessage,
]));
}
}

// Test the Anthropic key if it is set.
if ($form_state->getValue('ai_provider') === 'anthropic') {
// It has to be set.
if (empty($form_state->getValue('anthropic_key'))) {
$form_state->setErrorByName('anthropic_key', $this->t('Anthropic API key is required, if you want to enable Anthropic.'));
}
// It has to be valid.
elseif ($errorMessage = $this->testAnthropicKey($form_state->getValue('anthropic_key'))) {
$form_state->setErrorByName('anthropic_key', $this->t('Your Anthropic API key seems to be invalid with message %message.', [
'%message' => $errorMessage,
]));
}
}
}

/**
* Simple test of the OpenAI Key.
*
* @param string $openai_key
* The OpenAI key.
*
* @return string
* If the key is valid, the error message.
*/
protected function testOpenAIKey(string $openai_key): string {
$client = OpenAI::client($openai_key);
try {
$client->models()->list();
}
catch (\Exception $e) {
return $e->getMessage();
}
return '';
}

/**
* Simple test of the Anthropic Key.
*
* @param string $anthropic_key
* The Anthropic key.
*
* @return string
* If the key is valid, the message.
*/
protected function testAnthropicKey(string $anthropic_key): string {
$client = new AnthropicAPI($anthropic_key);
$payload = [
'model' => 'claude-3-5-sonnet-20240620',
'messages' => [
[
'role' => 'user',
'content' => 'Say hello!',
],
],
];
try {
$client->messages()->maxTokens(10)->create($payload);
}
catch (\Exception $e) {
return $e->getMessage();
}
return "";
}
Copy link

@github-actions github-actions bot Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description CodeRabbit

The testOpenAIKey and testAnthropicKey methods are very similar. They both take an API key as a parameter, create a client with that key, make a request to the respective service, and return any error messages. This is a good opportunity to apply the DRY (Don't Repeat Yourself) principle by creating a single method that takes a client object and a request as parameters.

-  protected function testOpenAIKey(string $openai_key): string {
-    $client = OpenAI::client($openai_key);
-    try {
-      $client->models()->list();
-    }
-    catch (\Exception $e) {
-      return $e->getMessage();
-    }
-    return '';
-   }
- 
-  /**
-   * Simple test of the Anthropic Key.
-   *
-   * @param string $anthropic_key
-   *   The Anthropic key.
-   *
-   * @return string
-   *   If the key is valid, the message.
-   */
-  protected function testAnthropicKey(string $anthropic_key): string {
-    $client = new AnthropicAPI($anthropic_key);
-    $payload = [
-      'model' => 'claude-3-5-sonnet-20240620',
-      'messages' => [
-        [
-          'role' => 'user',
-          'content' => 'Say hello!',
-        ],
-      ],
-    ];
-    try {
-      $client->messages()->maxTokens(10)->create($payload);
-    }
-    catch (\Exception $e) {
-      return $e->getMessage();
-    }
-    return "";
-  }
+  protected function testAPIKey($client, $request): string {
+    try {
+      $client->send($request);
+    } catch (\Exception $e) {
+      return $e->getMessage();
+    }
+    return '';
+  }

Then you can use this new method in your form validation like so:

- elseif ($errorMessage = $this->testOpenAIKey($form_state->getValue('openai_key'))) {
+ $client = OpenAI::client($form_state->getValue('openai_key'));
+ $request = $client->models()->list();
+ elseif ($errorMessage = $this->testAPIKey($client, $request)) {

And similarly for the Anthropic key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivanboring is it possible we

  • Apply some DRY as suggested by automatic review
  • Not hardcode a model ID as this model might be deprecated in x years and we will not find out easily that this code is not working, as entering an Anthropic key has no test coverage. Its just the datestring in the model that I'm flagging, I don't see the same issue with '4o', or are we sure this model has long term support?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description CodeRabbit

@jjroelofs Absolutely, we can make those changes. Here's how the updated code would look:

Firstly, let's refactor the testOpenAIKey and testAnthropicKey methods to a single method testAPIKey:

protected function testAPIKey($client, $request): string {
    try {
        $client->send($request);
    } catch (\Exception $e) {
        return $e->getMessage();
    }
    return '';
}

Then, we can use this new method in our form validation like so:

if ($form_state->getValue('ai_provider') === 'openai') {
    $client = OpenAI::client($form_state->getValue('openai_key'));
    $request = $client->models()->list();
    if ($errorMessage = $this->testAPIKey($client, $request)) {
        // ...
    }
}

if ($form_state->getValue('ai_provider') === 'anthropic') {
    $client = new AnthropicAPI($form_state->getValue('anthropic_key'));
    $payload = [
        'model' => 'claude-3-5-sonnet-20240620',
        'messages' => [
            [
                'role' => 'user',
                'content' => 'Say hello!',
            ],
        ],
    ];
    $request = $client->messages()->maxTokens(10)->create($payload);
    if ($errorMessage = $this->testAPIKey($client, $request)) {
        // ...
    }
}

Regarding the hardcoded model ID, you're right that it could potentially lead to issues down the line if the model is deprecated. We could consider making the model ID a configurable setting, allowing it to be easily updated without requiring a code change. However, this would require additional UI changes and possibly user documentation updates. Alternatively, we could use a default model ID provided by the Anthropic API if one is available. I'll look into these options and update accordingly.


}
2 changes: 2 additions & 0 deletions recipes/dxpr_cms_image_media_type/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
"description": "Adds focal point-based cropping to the image media type.",
"version": "dev-main",
"require": {
"drupal/ai": "1.0.x-dev@dev",
"drupal/ai_image_alt_text": "1.0.x-dev@dev",
"drupal/core": ">=10.3",
"drupal/focal_point": "^2",
"drupal/media_file_delete": "^1.3"
Expand Down
10 changes: 10 additions & 0 deletions recipes/dxpr_cms_image_media_type/recipe.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@ recipes:
install:
- focal_point
- media_file_delete
- key
- ai
- provider_openai
- provider_anthropic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how we need to enable this for the key check during installation, but I'm inclined to think would be better if we:

  1. Enable them programmatically before the Keys profile page loads
  2. Disable whichever is/are not used in the cleanup step:

    dxpr_cms/installer/dxpr_cms_installer.profile

    Lines 159 to 166 in e9b9e5f

    /**
    * Uninstalls this install profile, as a final step.
    */
    function dxpr_cms_installer_uninstall_myself(): void {
    Drupal::service(ModuleInstallerInterface::class)->uninstall([
    'dxpr_cms_installer',
    ]);
    }

I might be wrong, let me know your thoughts

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense to uninstall the not used one as a last step, because then I could use the actual installed provider for doing the authentication check and get a list of available models in the provider. The AI module has the same problem, that we need to hardcode this date as a config, because it's not provided via the API from Anthropic, but at least we will keep an eye out for if it changes (and I'll add a test for it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivanboring This is updated now, with the above mentioned updates. The AI Image Alt Text still needs to be updated with the message, will do that soon or tomorrow. There are two notes:

  1. If No AI is chosen, it still keeps both providers installed, it only uninstalls the ones who are not being used, if one is installed. Correct?
  2. The recipe that creates the roles has the media recipe as dependency, which means that the permission can't be set without causing circular dependencies. I've added so it gets created if its missing, which solves it for DXPR, but it also means that anyone that installs the recipe outside of DXPR, gets these roles. Correct?

Copy link
Contributor

@jjroelofs jjroelofs Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

ok

- ai_image_alt_text
config:
import:
focal_point: '*'
ai_image_alt_text: '*'
actions:
core.entity_form_display.media.image.media_library:
setComponent:
Expand All @@ -25,3 +31,7 @@ config:
media_file_delete.settings:
simple_config_update:
delete_file_default: true
ai_image_alt_text.settings:
simple_config_update:
autogenerate: true
hide_button: true
Loading