Skip to content

[ai-form-recognizer] modelName and FormTrainingClient testing refactor#12203

Merged
willmtemple merged 9 commits into
Azure:masterfrom
willmtemple:ai-form-recognizer/model-display-name
Nov 4, 2020
Merged

[ai-form-recognizer] modelName and FormTrainingClient testing refactor#12203
willmtemple merged 9 commits into
Azure:masterfrom
willmtemple:ai-form-recognizer/model-display-name

Conversation

@willmtemple
Copy link
Copy Markdown
Contributor

@willmtemple willmtemple commented Oct 30, 2020

This adds support for modelName to the convenience layer API. Reviewers who are interested in that should look at the api.md file, src, and samples. Sorry for all the noise related to testing.

In the process of adding tests for this feature, I also refactored the existing FormTrainingClient tests.

  • I removed most recognition tests from the training tests. Now we only do a simple recognition using a URL against the trained models. The other recognition content types are still tested in the FormRecognizerClient tests.
  • There is no more split between browser/node form training client tests, as URL is the only method of form submission that is used to validate the model.
  • All FormTrainingClient tests run using both API Key and AAD by default.
  • Added some utility functions to help with covering more test cases without repeating code (see: test/utils/matrix.ts).
  • More combinations of model training parameters are tested using these new utility functions.

Closes #11233

@willmtemple willmtemple added Client This issue points to a problem in the data-plane of the library. Cognitive - Form Recognizer labels Oct 30, 2020
@willmtemple willmtemple self-assigned this Oct 30, 2020
@@ -16,6 +16,7 @@ These sample programs show how to use the JavaScript client libraries for Azure
| [differentiateLabeledUnlabeled.js][differentiatelabeledunlabeled] | See the differences in output when using a custom model trained with labeled data and one trained with unlabeled data |
| [copyModel.js][copymodel] | Copy a custom model from one Form Recognizer resource to another |
| [authenticationMethods.js][authenticationmethods] | authenticates a service client using both Azure Active Directory and an API key |
| [deleteAllModels.js][deleteallmodels] | Delete all models in a Form Recognizer account |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

curious about this sample since I don't think other languages have it. Is there a reason for adding?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Keyvault has one here in JS, and I've found working with this service that I really needed this code on hand to clean up my own FR account. I figured if I needed it, someone else probably will as well.

Copy link
Copy Markdown
Contributor

@kristapratico kristapratico left a comment

Choose a reason for hiding this comment

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

LGTM

const dotenv = require("dotenv");
dotenv.config();

async function main() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I love how easy and simple it looks for both JS and Python :)

/**
* An optional name to associate with the model
*/
modelName?: string;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in .NET we are moving this out of the option bag and leaving it as a parameter of BeginTraining becuase we expect customer to use model name frequently.
Could this be the case for JS too?

@willmtemple
Copy link
Copy Markdown
Contributor Author

/azp run js - formrecognizer - tests

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

*/

export function makeCredential(useAad: boolean): TokenCredential | AzureKeyCredential {
// if (isPlaybackMode()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: remove instead of commenting out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch thank you, I forgot to clean up some debugging changes to the utils.

* For strong type-checking, it is important that the `matrix` have a strong
* type, such as a `const` literal.
*
* @param values jagged 2D array specifying the arguments and their possible
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am just curious what happens if higher dimensional arrays are passed to matrix()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Anything that is nested deeper than 2 will become an argument itself.

matrix([
  [["a", "b", "c"], ["x", "y", "z"]],
  [1, 2, 3]
] as const, (arr: string[], n: number) => {
  // ...
}); 

Copy link
Copy Markdown
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Looks great! Love the test cleanup. I can see matrix being useful in other scenarios e.g., testing multi service version support.

@willmtemple
Copy link
Copy Markdown
Contributor Author

@jeremymeng Thanks! I think if we were going to have a shared utility library for tests, matrix would be a good candidate to add as well as anything that helps with our standard testing patterns.

@willmtemple willmtemple merged commit edbabf2 into Azure:master Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. Cognitive - Form Recognizer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ai-form-recognizer[ Model Display Name

5 participants