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

[formrecognizer] reduce time for recorded tests runs #11970

Merged
merged 12 commits into from
Jun 15, 2020
Merged

[formrecognizer] reduce time for recorded tests runs #11970

merged 12 commits into from
Jun 15, 2020

Conversation

kristapratico
Copy link
Member

@kristapratico kristapratico commented Jun 11, 2020 •
edited
Loading

Resolves #11932

Reduce the time our recorded tests runs take by setting polling_interval=0 if not running live.
Pipeline run times decrease from about 35 mins to 8 mins with this PR.

This PR ended up with a few things:

  • Refactor testcase.py and add GlobalClientPreparer - this is so we can create the client for each test in the preparer and set the polling_interval accordingly here instead of passing that keyword into each method in each test
  • Sets the default polling interval (5s) at the client-level now, and allows user override at the client or operation-level
  • Adds tests for polling interval for FormRecognizerClient and FormTrainingClient
  • Adds a transport wrapper for the get_client method b/c we weren't sharing the pipeline between the clients previously. @rakshith91 - can you look at this part? I remember you doing this work for storage as well.

Live test pass: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=422700&view=results

@kristapratico kristapratico added Cognitive Services Client This issue points to a problem in the data-plane of the library. FormRecognizer labels Jun 11, 2020
@kristapratico kristapratico self-assigned this Jun 11, 2020
@kristapratico kristapratico marked this pull request as ready for review June 12, 2020 20:20
@kristapratico kristapratico requested a review from rakshith91 June 12, 2020 20:21
iscai-msft
iscai-msft previously approved these changes Jun 12, 2020
View reviewed changes
sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_form_recognizer_client.py
@@ -66,11 +66,13 @@ def __init__(self, endpoint, credential, **kwargs):
# type: (str, Union[AzureKeyCredential, TokenCredential], Any) -> None

authentication_policy = get_authentication_policy(credential)
polling_interval = kwargs.pop("polling_interval", POLLING_INTERVAL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we documenting this polling_interval kwarg to the client for users? I think a good idea could be to call it _polling_interval if we're the only people expecting the use it, but I don't feel too strongly about this

Copy link
Member Author

@kristapratico kristapratico Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is an azure-core setting, so hopefully once Xiang documents all the core keywords we'll be able to link to that :)

Edit: this should be available to users

iscai-msft
iscai-msft previously approved these changes Jun 15, 2020
View reviewed changes
rakshith91
rakshith91 reviewed Jun 15, 2020
View reviewed changes
sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_form_training_client.py Outdated
Comment on lines 383 to 384
self._endpoint,
self._credential,
Copy link
Contributor

@rakshith91 rakshith91 Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

minor nit:

Suggested change
self._endpoint,
self._credential,
endpoint=self._endpoint,
crendential=self._credential,

rakshith91
rakshith91 reviewed Jun 15, 2020
View reviewed changes
Copy link
Contributor

@rakshith91 rakshith91 left a comment

Choose a reason for hiding this comment

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

Looks good - but please add tests for the transport wrapper.

You can refer these:

azure-sdk-for-python/sdk/storage/azure-storage-blob/tests/test_common_blob.py

Line 2005 in 379ae6e

def test_transport_closed_only_once(self, resource_group, location, storage_account, storage_account_key):

rakshith91
rakshith91 previously approved these changes Jun 15, 2020
View reviewed changes
@kristapratico kristapratico dismissed stale reviews from rakshith91 and iscai-msft via c60894e June 15, 2020 19:14
@kristapratico
Copy link
Member Author

Looks good - but please add tests for the transport wrapper.

You can refer these:

azure-sdk-for-python/sdk/storage/azure-storage-blob/tests/test_common_blob.py

Line 2005 in 379ae6e

def test_transport_closed_only_once(self, resource_group, location, storage_account, storage_account_key):

we do have tests, discussed offline

@kristapratico kristapratico merged commit 010bc93 into Azure:master Jun 15, 2020
@kristapratico kristapratico deleted the fr-recorded-tests branch June 15, 2020 19:31
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jun 17, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
6585bef 
…into regenerate_keys

* 'master' of https://github.com/Azure/azure-sdk-for-python: (26 commits)
  [formrecognizer] update formrecognizer links to new aka.ms naming (Azure#12079)
  changes in samples tests (Azure#12090)
  readme & sample updates (Azure#12095)
  Update Key Vault minimum azure-core to 1.4.0 (Azure#12074)
  [formrecognizer] test parity with other languages (Azure#12059)
  syncing missing changelog items (Azure#12089)
  updating doc references (Azure#12086)
  reserve 1 more version for storage and network (Azure#12082)
  Fix format in swagger_to_sdk_config.json (Azure#12083)
  modify changelog (Azure#12071)
  Update Cosmos CODEOWNERS (Azure#11500)
  Regenerate LUIS (Azure#12064)
  Enable track2 SDK Automation config on master branch (Azure#11654)
  Update KeyVaultPreparer with track 2 mgmt changes (Azure#12060)
  Increment version for storage releases (Azure#12034)
  AzureCliCredential correctly invokes /bin/sh (Azure#12056)
  [formrecognizer] reduce time for recorded tests runs (Azure#11970)
  disable some bandit warnings (Azure#12054)
  Respect nbf and exp in local encrypt/wrap operations (Azure#11953)
  add bug_bash template (Azure#12045)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jun 17, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
1b9a7c4 
…into regenerate_certs

* 'master' of https://github.com/Azure/azure-sdk-for-python: (21 commits)
  [formrecognizer] update formrecognizer links to new aka.ms naming (Azure#12079)
  changes in samples tests (Azure#12090)
  readme & sample updates (Azure#12095)
  Update Key Vault minimum azure-core to 1.4.0 (Azure#12074)
  [formrecognizer] test parity with other languages (Azure#12059)
  syncing missing changelog items (Azure#12089)
  updating doc references (Azure#12086)
  reserve 1 more version for storage and network (Azure#12082)
  Fix format in swagger_to_sdk_config.json (Azure#12083)
  modify changelog (Azure#12071)
  Update Cosmos CODEOWNERS (Azure#11500)
  Regenerate LUIS (Azure#12064)
  Enable track2 SDK Automation config on master branch (Azure#11654)
  Update KeyVaultPreparer with track 2 mgmt changes (Azure#12060)
  Increment version for storage releases (Azure#12034)
  AzureCliCredential correctly invokes /bin/sh (Azure#12056)
  [formrecognizer] reduce time for recorded tests runs (Azure#11970)
  disable some bandit warnings (Azure#12054)
  Respect nbf and exp in local encrypt/wrap operations (Azure#11953)
  add bug_bash template (Azure#12045)
  ...
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Dec 15, 2020
CodeGen from PR 11970 in Azure/azure-rest-api-specs
1467970 
add hybridcompute track2 config (Azure#11970)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iscai-msft iscai-msft iscai-msft approved these changes

@rakshith91 rakshith91 rakshith91 left review comments

Assignees

@kristapratico kristapratico

Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Form Recognizer Cognitive Services
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[formrecognizer] look into speeding up recorded runs
3 participants
@kristapratico @rakshith91 @iscai-msft