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

[text analytics] change return type of analyze actions to list of list #18994

Merged
merged 5 commits into from
Jun 14, 2021
Merged

[text analytics] change return type of analyze actions to list of list #18994

merged 5 commits into from
Jun 14, 2021

Conversation

iscai-msft
Copy link
Contributor

Instead of adding a new class AnalyzeActionsResult, we're going to reuse the current action result classes, i.e. RecognizeEntitiesResult.

Now, a call with documents ["Hello", "world"] and actions [RecognizeEntitiesAction(), AnalyzeSentimentAction()] will get you
[
[RecognizeEntitiesResult of "Hello", AnalyzeSentimentResult of "Hello"],
[RecognizeEntitiesResult of "world", AnalyzeSentimentResult of "world"],
]

This is more consistent with the return types of the normal operations, and allows us to use the models we currently have

@iscai-msft iscai-msft requested a review from johanste May 27, 2021 22:34
kristapratico
kristapratico approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member

@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.

nits

sdk/textanalytics/azure-ai-textanalytics/CHANGELOG.md Outdated Show resolved Hide resolved
sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze.py Show resolved Hide resolved
sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze.py Outdated Show resolved Hide resolved
sdk/textanalytics/azure-ai-textanalytics/tests/test_analyze_async.py Outdated Show resolved Hide resolved
sdk/textanalytics/azure-ai-textanalytics/tests/testcase.py Outdated Show resolved Hide resolved
johanste
johanste reviewed Jun 1, 2021
View reviewed changes
sdk/textanalytics/azure-ai-textanalytics/README.md Outdated
for doc, document_results in zip(documents, pages):
print("\nDocument text: {}".format(doc))
recognize_entities_result = document_results[0]
assert not recognize_entities_result.is_error
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 probably not a good use of an assert. Any document could have failed for a transient reason, so you are not really in a position to say that this didn't fail. I would print a "failed" message instead...

Copy link
Member

Choose a reason for hiding this comment

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

Actually, scratch that, this is in the readme.md. It is a bit long for the readme already. I don't think we need to show all types of actions in the readme example inline. It definitely won't scale as new action types comes along. Can we limit this to two?

@iscai-msft iscai-msft requested a review from johanste June 1, 2021 17:22
iscai-msft added 2 commits June 1, 2021 13:25
@iscai-msft
limit # of action calls in readme example
86f8294
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
f192e29 
…into update_analyze_output

* 'master' of https://github.com/Azure/azure-sdk-for-python: (123 commits)
  [text analytics] Fix some issues i've found (Azure#19081)
  [Key Vault] Add API version 7.2 for administration (Azure#18997)
  hide to_analyze_request (Azure#19079)
  Add environment variable for redirecting IMDS token requests (Azure#18967)
  [translation] poller design (Azure#19041)
  [AutoRelease] t2-resourcemover-2021-05-21-54304(wave4) (Azure#18849)
  [AutoRelease] t2-logz-2021-06-02-49354 (Azure#19035)
  prepare pipeline (Azure#19062)
  [AutoRelease] t2-recoveryservicesbackup-2021-05-26-33193 (Azure#18953)
  Get rid of DataFeedOptions (Azure#19054)
  [AutoRelease] t2-batchai-2021-06-02-38609 (Azure#19036)
  [appconfig] new gen code (Azure#18859)
  [Tables] mypy (Azure#19001)
  fix misleading url (Azure#19044)
  Create py.typed (Azure#19052)
  add override of opentelemetry-api to 1.3.0 (Azure#19048)
  Prepare for June Release (Azure#19043)
  [Key Vault] Add API version 7.2 for keys (Azure#18586)
  some misc fixes. (Azure#19024)
  [AppConfig] pre release (Azure#19027)
  ...
@kristapratico kristapratico mentioned this pull request Jun 10, 2021
Closed
5 tasks
@kristapratico kristapratico merged commit 407d0b7 into Azure:master Jun 14, 2021
iscai-msft added a commit that referenced this pull request Jun 16, 2021
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
b03039a 
…into protocol_base

* 'master' of https://github.com/Azure/azure-sdk-for-python: (229 commits)
  Sync eng/common directory with azure-sdk-tools for PR 1688 (#19272)
  update all ubuntu vmImage to 20.04 (#19227)
  add base class for feedback (#19265)
  Enable tests for integration samples (#18531)
  docs: fix a few simple typos (#19127)
  Increment package version after release of azure-eventgrid (#19197)
  Increment package version after release of azure-monitor-query (#19208)
  earliest versions of communication identity tests are set up improperly. skip 1.0.0 in regression testing (#19258)
  Run mypy in azure-keyvault-administration CI (#19246)
  [text analytics] change return type of analyze actions to list of list (#18994)
  Increment package version after release of azure-data-tables (#19192)
  suppress min number less than 14 (#19230)
  [Key Vault] Address administration feedback (#19099)
  [textanalytics] remove warning tests (#19217)
  [translation] enable samples to run in CI (#19190)
  update custom_granularity_value to be more than 300 (#19215)
  Increment package version after release of azure-security-attestation (#19212)
  Enabling Before/After Test Customization for Standard CI Pipelines (#19187)
  bump sphinx version to resolve the weird issue with the caption not getting the correct class (#19188)
  [AutoRelease] t2-agrifood-2021-06-09-15946 (#19174)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jun 17, 2021
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
fb692df 
…into get_testserver_working

* 'master' of https://github.com/Azure/azure-sdk-for-python: (55 commits)
  [AppConfig] mypy fixes (Azure#18858)
  [translation] renames (Azure#19285)
  [Key Vault] Update metadata for release (Azure#19286)
  [AppConfig] enable samples (Azure#18857)
  KeyVaultBackupOperation -> KeyVaultBackupResult (Azure#19284)
  renaming (Azure#19280)
  [Key Vault] Custom polling method for admin backup client (Azure#19204)
  [textanalytics] add ARM template + run samples in CI (Azure#19270)
  removed example from description (Azure#18781)
  [Key Vault] Update READMEs with RBAC information (Azure#19275)
  Sync eng/common directory with azure-sdk-tools for PR 1688 (Azure#19272)
  update all ubuntu vmImage to 20.04 (Azure#19227)
  add base class for feedback (Azure#19265)
  Enable tests for integration samples (Azure#18531)
  docs: fix a few simple typos (Azure#19127)
  Increment package version after release of azure-eventgrid (Azure#19197)
  Increment package version after release of azure-monitor-query (Azure#19208)
  earliest versions of communication identity tests are set up improperly. skip 1.0.0 in regression testing (Azure#19258)
  Run mypy in azure-keyvault-administration CI (Azure#19246)
  [text analytics] change return type of analyze actions to list of list (Azure#18994)
  ...
@iscai-msft iscai-msft deleted the update_analyze_output branch September 10, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristapratico kristapratico kristapratico approved these changes

@johanste johanste johanste approved these changes

@abhahn abhahn Awaiting requested review from abhahn

Assignees
No one assigned
Labels
Cognitive - Text Analytics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iscai-msft @johanste @kristapratico