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

[FormRecognizer] Test fix and sample reordering #11728

Merged
merged 8 commits into from
May 4, 2020

Conversation

kinelski
Copy link
Member

@kinelski kinelski commented May 1, 2020

Set of changes:

  • Fixed Ignored sample tests.
  • Moved common functionality from FormRecognizerClientLiveTests and FormTrainingClientLiveTests to the new class FormRecognizerLiveTestBase.
  • Reordered samples to make training appear before custom recognition.
  • "Train Model with Forms" and "Train Model with Forms and Labels" samples numbering updated to be the same.

Fixes #11493

@kinelski kinelski self-assigned this May 1, 2020
@kinelski kinelski added Client This issue points to a problem in the data-plane of the library. Cognitive Services Docs FormRecognizer labels May 1, 2020
@kinelski kinelski added this to the [2020] May milestone May 1, 2020
@kinelski
Copy link
Member Author

kinelski commented May 1, 2020

/azp run net - formrecognizer - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kinelski kinelski requested a review from annelo-msft May 1, 2020 16:26
@kinelski kinelski marked this pull request as ready for review May 1, 2020 16:27
@kinelski kinelski requested a review from maririos as a code owner May 1, 2020 16:27
@annelo-msft
Copy link
Member

We worked with the other languages decide which samples to expose in what order. I don't think it make sense to change these around and realign with other languages this close to the next release.

@maririos
Copy link
Member

maririos commented May 4, 2020

Reordered samples to make training appear before custom recognition.

The agreement across languages was to leave custom before training. Users will use custom recognition more often than training. Also, we are expecting for the majority to use the Labeling Tool.

"Train Model with Forms" and "Train Model with Forms and Labels" samples numbering updated to be the same.

Numbering goes according to complexity in the sample. I left them with different numbers because "with forms and labels" implies that you need extra documents with the labeling format and therefore is a way more complicated scenario

@kinelski
Copy link
Member Author

kinelski commented May 4, 2020

We worked with the other languages decide which samples to expose in what order.

The agreement across languages was to leave custom before training.

I didn't work directly with samples in the last preview, so I wasn't aware it was an agreement across languages. I'm not really fond of the custom => training order, but I agree that deviating from other languages at this point would not be profitable. I'll revert it back.

@kinelski
Copy link
Member Author

kinelski commented May 4, 2020

Numbering goes according to complexity in the sample. I left them with different numbers because "with forms and labels" implies that you need extra documents with the labeling format and therefore is a way more complicated scenario

I was thinking more about the "code complexity" than the "scenario complexity" itself, since the code of the samples are basically the same, but I don't really mind that much. I think your approach is reasonable as well.

I was a bit concerned about having a Sample4_TrainModel.md file for both Sample4_TrainModelWithForms.cs and Sample5_TrainModelWithFormsAndLabels.cs,

while having Sample5_ManageCustomModels.md for Sample6_ManageCustomModels.cs, but maybe I'm just overconcerning.

I'll revert it back since it's not really this PR's focus. We can always discuss it again if we think it's a reason for concern.

@kinelski
Copy link
Member Author

kinelski commented May 4, 2020

/azp run net - formrecognizer - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kinelski
Copy link
Member Author

kinelski commented May 4, 2020

/azp run net - formrecognizer - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kinelski kinelski merged commit e554e75 into Azure:master May 4, 2020
@kinelski kinelski deleted the fr-tests branch May 4, 2020 23:17
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-net that referenced this pull request Nov 26, 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 Cognitive Services Docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure sample test run and pass in our nightly builds
3 participants