-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[text analytics] Analyze updates for v5.1.0b6 #17003
Conversation
sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_models.py
Outdated
Show resolved
Hide resolved
@@ -804,6 +805,10 @@ def begin_analyze_batch_actions( # type: ignore | |||
key_phrase_extraction_tasks=[ | |||
t.to_generated() for t in | |||
[a for a in actions if _determine_action_type(a) == AnalyzeBatchActionsType.EXTRACT_KEY_PHRASES] | |||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update the typehint on line 734?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I also had to disable line-too-long
for pylint on this one.
@@ -54,13 +54,11 @@ def analyze_status( | |||
:type skip: int | |||
:keyword callable cls: A custom type or function that will be passed the direct response | |||
:return: AnalyzeJobState, or the result of cls(response) | |||
:rtype: ~azure.ai.textanalytics.v3_1_preview_3.models.AnalyzeJobState | |||
:rtype: ~azure.ai.textanalytics.v3_1_preview_4.models.AnalyzeJobState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a changelog for this addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Let me know if there's anything missing there.
], | ||
entity_linking_tasks=[ | ||
t.to_generated() for t in | ||
[a for a in actions if _determine_action_type(a) == AnalyzeBatchActionsType.RECOGNIZE_LINKED_ENTITIES] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add some tests for linked entities and update the samples (readme and actual samples)? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was working on this, just ran out of time yesterday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've updated tests and samples. I've deleted a few recordings in preparation for running them, but I don't think this has been deployed to WestUS2 yet so I will wait to re-run the tests for now. In the meantime let me know if there are any other tests I should modify/add.
… rerunning a few tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
barring the diff in the generated code, rest looks good. thanks @abhahn !
@@ -139,7 +139,7 @@ async def begin_health( | |||
self, | |||
documents: List["_models.MultiLanguageInput"], | |||
model_version: Optional[str] = None, | |||
string_index_type: Optional[Union[str, "_models.StringIndexType"]] = "TextElements_v8", | |||
string_index_type: Optional[Union[str, "_models.StringIndexType"]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhan why is the default here None
, it should still be TextElements_v8
…into ta-v5.1.0b6-analyze * 'master' of https://github.com/Azure/azure-sdk-for-python: (24 commits) [text analytics] PII updates for v5.1.0b6 (Azure#17038) Fix bug where imported matrix parameter duplicates are not overrided (Azure#17126) Add NULL to readme (Azure#17076) Sas batching (Azure#17133) dropping py3.5 (Azure#17127) [EventHubs] 5.3.1 update changelog (Azure#17132) [text analytics] assertions (Azure#17098) add recordings (Azure#17125) [core] add HttpRequest and HttpResponse reprs (Azure#16972) [text analytics] add actual normalized_text tests (Azure#17123) update samples to use actual role names (Azure#17124) Sync eng/common directory with azure-sdk-tools for PR 1448 (Azure#17085) Enable APIView status check (Azure#17107) Fix PackageName typo (Azure#17109) Move SetTestPipelineVersion.ps1 to eng/common (Azure#17103) Fix paths for non-windows agents (Azure#17096) [Communication] - Identity - Update README (Azure#17091) Rename CertificateCredential's certificate_bytes -> certificate_data (Azure#17090) fix shared reqs (Azure#17095) [translation] initial library (Azure#16837) ...
…-python into ta-v5.1.0b6-analyze
Hello @iscai-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
…into update_ta_tests * 'master' of https://github.com/Azure/azure-sdk-for-python: Update get_package_properties.py logic for python 2.7 (#17144) update changelog (#17150) [ServiceBus] 7.1.0 Release update changelog (#17135) [ServiceBus] Object mapping support (#17080) move SetTestPipeline into its own template (#17141) Revise token cache configuration API (#16326) Fix dup cloud error (#17097) Perf tests for monitor exporter (#17067) [Communication] - Phone Number - Redesigned API (#16671) disable retry (#17078) [Key Vault] Add perf tests for certificates, keys, and secrets (#17073) [text analytics] Analyze updates for v5.1.0b6 (#17003) Add any additional claims to AuthenticationRequiredError (#17136) Fix logic in SetTestPipelineVersionInEngCommon (#17138) [Key Vault] Make test resource cleanup script asynchronous (#17032)
fixes #16372