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

Add Azure Language Services provider #786

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

psorensen
Copy link

@psorensen psorensen commented Aug 1, 2024

Description of the Change

This PR adds Azure Language Services as a provider and adds it to the Excerpt Generation feature.

Closes #159

How to test the Change

Testing Steps:

  1. Create new Azure resource for Language Services
  2. Enter Credentials into the Excerpt Generation feature screen
  3. Confirm valid credentials allow feature to be enabled
  4. Test 'Generate Excerpt' tag for allowed post types.

Changelog Entry

Added - Azure Language Services provider with Excerpt Generation.

Credits

Props @psorensen @cadic @jeffpaul @Sidsector9 @dkotter

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@psorensen psorensen requested review from dkotter, jeffpaul and a team as code owners August 1, 2024 18:47
@github-actions github-actions bot added this to the 3.2.0 milestone Aug 1, 2024
@github-actions github-actions bot added the needs:feedback This requires reporter feedback to better understand the request. label Aug 1, 2024
Copy link

github-actions bot commented Aug 1, 2024

@psorensen thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this?

@psorensen psorensen marked this pull request as draft August 1, 2024 19:18
@github-actions github-actions bot removed the needs:feedback This requires reporter feedback to better understand the request. label Aug 1, 2024
@psorensen psorensen force-pushed the feature/azure-language-service branch from 1239c7d to 55bca24 Compare August 2, 2024 13:48
@psorensen psorensen marked this pull request as ready for review August 5, 2024 20:17
@github-actions github-actions bot added the needs:feedback This requires reporter feedback to better understand the request. label Aug 5, 2024
@psorensen
Copy link
Author

@dkotter How should I handle the VIP flag for wp_remote_get usage? Should I conditionally use it if defined, or ignore this rule?

@dkotter
Copy link
Collaborator

dkotter commented Aug 5, 2024

@dkotter How should I handle the VIP flag for wp_remote_get usage? Should I conditionally use it if defined, or ignore this rule?

So we've not been consistent with this so far in the codebase. We've basically done both, where we use vip_safe_wp_remote_get if it exists (see here) and otherwise use wp_remote_get and in other places we just use wp_remote_get and add an ignore statement.

Personally I'd prefer we go with the first approach, as this is technically a VIP approved plugin so it's nice to match their standards. We have had issues though a few times with timeouts, as some of these services take a bit to return. vip_safe_wp_remote_get only allows you to set a timeout of 3 seconds so if we need higher than that here, we'll want to stick with wp_remote_get only

@psorensen psorensen force-pushed the feature/azure-language-service branch from 1bac627 to b550389 Compare August 6, 2024 00:39
@github-actions github-actions bot added needs:code-review This requires code review. and removed needs:feedback This requires reporter feedback to better understand the request. labels Aug 6, 2024
@psorensen
Copy link
Author

sounds good @dkotter - I've included a helper function to facilitate a refactor of all the existing usage of wp_remote_get

@jeffpaul jeffpaul removed request for a team and jeffpaul August 13, 2024 02:22
@jeffpaul
Copy link
Member

@psorensen is this still WIP or can this formally go through review/merge?

@psorensen psorensen changed the title WIP: Add Azure Language Services provider Add Azure Language Services provider Aug 13, 2024
@psorensen
Copy link
Author

@jeffpaul Good to review - removed the WIP from the title

$endpoint = trailingslashit( $url ) . '/text/analytics/v3.1/languages';
$endpoint = add_query_arg( 'api-version', static::API_VERSION, $endpoint );

$request = safe_wp_remote_get(
Copy link
Member

Choose a reason for hiding this comment

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

This is throwing error. Should it use wp_remote_post() instead?

Copy link
Author

Choose a reason for hiding this comment

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

See this comment and the follow-up comments for context. What error are you seeing?

Copy link
Member

Choose a reason for hiding this comment

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

I am seeing the following when trying to connect to the service:

Screenshot 2024-09-06 at 2 10 22 PM

Copy link
Member

Choose a reason for hiding this comment

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

Here's the neater version:

[06-Sep-2024 08:40:15 UTC] PHP Fatal error:  Uncaught TypeError: http_build_query(): Argument #1 ($data) must be of type array, string given in /Users/work/Sites/classifai/app/public/wp-includes/Requests/src/Transport/Curl.php:578
Stack trace:
#0 /Users/work/Sites/classifai/app/public/wp-includes/Requests/src/Transport/Curl.php(578): http_build_query('{"documents": [...', '', '&')
#1 /Users/work/Sites/classifai/app/public/wp-includes/Requests/src/Transport/Curl.php(391): WpOrg\Requests\Transport\Curl::format_get('https://classif...', '{"documents": [...')
#2 /Users/work/Sites/classifai/app/public/wp-includes/Requests/src/Transport/Curl.php(171): WpOrg\Requests\Transport\Curl->setup_handle('https://classif...', Array, '{"documents": [...', Array)
#3 /Users/work/Sites/classifai/app/public/wp-includes/Requests/src/Requests.php(469): WpOrg\Requests\Transport\Curl->request('https://classif...', Array, '{"documents": [...', Array)
#4 /Users/work/Sites/classifai/app/public/wp-includes/class-wp-http.php(397): WpOrg\Requests\Requests::request('https://classif...', Array, '{"documents": [...', 'GET', Array)
#5 /Users/work/Sites/classifai/app/public/wp-includes/class-wp-http.php(638): WP_Http->request('https://classif...', Array)
#6 /Users/work/Sites/classifai/app/public/wp-includes/http.php(184): WP_Http->get('https://classif...', Array)
#7 /Users/work/Sites/classifai/app/public/wp-content/plugins/classifai/includes/Classifai/Helpers.php(686): wp_remote_get('https://classif...', Array)
#8 /Users/work/Sites/classifai/app/public/wp-content/plugins/classifai/includes/Classifai/Providers/Azure/Language.php(164): Classifai\safe_wp_remote_get('https://classif...', Array)
#9 /Users/work/Sites/classifai/app/public/wp-content/plugins/classifai/includes/Classifai/Providers/Azure/Language.php(122): Classifai\Providers\Azure\Language->authenticate_credentials('https://classif...', '95193e52eec647f...')
#10 /Users/work/Sites/classifai/app/public/wp-content/plugins/classifai/includes/Classifai/Features/Feature.php(258): Classifai\Providers\Azure\Language->sanitize_settings(Array)
#11 /Users/work/Sites/classifai/app/public/wp-includes/class-wp-hook.php(326): Classifai\Features\Feature->sanitize_settings(Array)
#12 /Users/work/Sites/classifai/app/public/wp-includes/plugin.php(205): WP_Hook->apply_filters(Array, Array)
#13 /Users/work/Sites/classifai/app/public/wp-includes/formatting.php(5115): apply_filters('sanitize_option...', Array, 'classifai_featu...', Array)
#14 /Users/work/Sites/classifai/app/public/wp-includes/option.php(866): sanitize_option('classifai_featu...', Array)
#15 /Users/work/Sites/classifai/app/public/wp-admin/options.php(344): update_option('classifai_featu...', Array)
#16 {main}
  thrown in /Users/work/Sites/classifai/app/public/wp-includes/Requests/src/Transport/Curl.php on line 578

}

while ( 'succeeded' !== $response->status ) {
sleep( .5 );
Copy link
Member

Choose a reason for hiding this comment

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

A downside of this approach is that if you navigate away from the page, the process will quit.
We can use Action Scheduler for background processing which we're using for the Classification feature. (#779)

@dkotter do you have any thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the problem here just in quickly looking at the code is that this method gets called from a REST request. The easy fix is what we're doing here, just pausing and retrying until we get a successful response.

I do wonder what happens if the response never succeeds? Would this code just be stuck in this loop?

Action Scheduler would be a nicer fix here but would be more complicated with how everything else is set up in ClassifAI. Basically when the Generate Excerpt button is clicked, the initial request would trigger Action Scheduler and then on the javascript side, we would need to continue polling until the request is done (and show the loading indicator the entire time).

That's doable but doesn't match how we handle things elsewhere so would need to think through how best to achieve that, especially in a way that would work for other Features and Providers in the future

* @param string $summary_length The summary length.
* @return string
*/
'summaryLength' => apply_filters( 'classifai_azure_language_summary_length', 'oneSentence' ),
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to format the doc block

/**
 * Filter the summary length.
 * Possible values are 'oneSentence', 'short', 'medium', 'long'.
 * Default is 'oneSentence'.
 *
 * @since 3.2.0
 * @hook classifai_azure_language_summary_length
 * @param {string} $summary_length The summary length.
 * @return {string} The summary length
 */

$response = json_decode( wp_remote_retrieve_body( $request ) );
}

$summary = $response->tasks->items[0]->results->documents[0]->summaries[0]->text;
Copy link
Member

Choose a reason for hiding this comment

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

Just want to confirm that the array indexes at [0] will always contain those properties, or if we should check via isset() and count() > 0?

Copy link

@psorensen thanks for the PR! Could you please rebase your PR on top of the latest changes in the base branch?

@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review. needs:refresh This requires a refreshed PR to resolve.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-populate missing meta tags and descriptions
4 participants