[translation] poller design by kristapratico · Pull Request #19041 · Azure/azure-sdk-for-python

kristapratico · 2021-06-02T17:17:04Z

Resolves #17915

Models translation as an LRO. Try not to focus on the names too much, will update in a separate PR.

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

kristapratico · 2021-06-02T17:19:16Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/_client.py

+        continuation_token = kwargs.pop("continuation_token", None)
+        pipeline_response = None
+        if continuation_token:
+            pipeline_response = self._client.document_translation.get_translation_status(


this is weird I know, but necessary for us to use the job ID as the continuation_token instead of pickling the whole response

kristapratico · 2021-06-02T20:07:17Z

/azp run python - core - ci

azure-pipelines · 2021-06-02T20:07:35Z

Azure Pipelines successfully started running 1 pipeline(s).

maririos

Reviewed in general. no specifics on Python implementation for LRO

sdk/translation/azure-ai-translation-document/CHANGELOG.md

maririos · 2021-06-02T20:17:09Z

sdk/translation/azure-ai-translation-document/CHANGELOG.md

+to wait until the translation is complete. See the README for more information about LROs.
+- Upon completion of the LRO, `begin_translation` now returns a pageable of `DocumentStatusResult`. All job-level metadata can still
+be found on `poller.details`.
+- `has_completed` has been removed from `JobStatusResult` and `DocumentStatusResult`. Use `poller.done()` to check if the 


ohhh interesting. So if a user wants to know if a specific document is done, they will have to check the status of the document and "calculate it" or just wait until the poller finishes, right?
why?

People found this confusing in the user study - they thought it was a dynamic property that would update the status in python. Moving to the poller allows us to actually have that dynamic property with done(). I feel it's less important to have on DocumentStatusResult, at that point you're actually interested in checking what the status is - "failed", "succeeded" etc to see what to do (at least that's how all our samples are written). Checking with johan on this one.

Johan said to remove it

sdk/translation/azure-ai-translation-document/CHANGELOG.md

sdk/translation/azure-ai-translation-document/README.md

maririos · 2021-06-02T20:27:33Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/_models.py

    :ivar int documents_not_yet_started_count: Number of documents that have not yet started being translated.
    :ivar int documents_cancelled_count: Number of documents that were cancelled for translation.
    :ivar int total_characters_charged: Total characters charged across all documents within the job.
-    :ivar bool has_completed: boolean to check whether a job has finished or not.


The JobStatusResult is used when the user calls list_submitted_jobs right? I don't fully understand why the property is gone after we saw in the user studies that it was helpful to have it

Let me check the UX study notes - I remember the opposite feedback (at least for Python)

interesting. I remember .NET people always using that property.
I also remember they confuse it with the HasCompleted from the operation so I guess we could remove it and if people complain then added back.
I don't feel strongly about either option

oh interesting, didn't realize you also had a hasCompleted on the operation. Yeah, I checked the notes on the understanding of has_completed and there were 4 no and two maybes. Guessing those are mostly python participants 😄

maririos · 2021-06-02T20:28:33Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/_models.py

        Value is between [0.0, 1.0].
    :ivar str id: Document Id.
    :ivar int characters_charged: Characters charged for the document.
-    :ivar bool has_completed: boolean to check whether a document finished translation or not.


same comment here

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/_polling.py

maririos · 2021-06-02T20:36:50Z

sdk/translation/azure-ai-translation-document/tests/asynctestcase.py

            )

-    async def _submit_and_validate_translation_job_async(self, async_client, translation_inputs, total_docs_count=None):
+    async def _begin_and_validate_translation_async(self, async_client, translation_inputs, total_docs_count=None, language=None):


in which cases you will get here and not have documents to translate?

and when is language none? sorry trying to understand the tests

These are just options to pass if you want to validate certain properties. Let me look a little closer, it might need a bit of a refactor.

maririos

Overall design LGTM (no specific Python review)

iscai-msft · 2021-06-02T22:50:17Z

...lation/azure-ai-translation-document/samples/async_samples/sample_begin_translation_async.py

            ]
-        )  # type: JobStatusResult
+        )
+        result = poller.result()


i think you have to await this too

thanks for catching. Also opened an issue to start running samples in the CI, would have caught that

…into fix_paging_types * 'master' of https://github.com/Azure/azure-sdk-for-python: (37 commits) [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) [ACR] prerelease (Azure#19028) Add sdk/core to triggers and pr (Azure#19023) Event grid 4.3.0 regen code (Azure#19025) Handle 6 decimal places cloud event (Azure#19019) ...

…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 added 4 commits June 1, 2021 20:33

add polling implementation

7f187e5

add polling generation transforms and regenerate

7003ea4

add tests and recordings

c50e949

samples, readme, changelog

9016aff

kristapratico added the Cognitive - Translator label Jun 2, 2021

kristapratico self-assigned this Jun 2, 2021

kristapratico commented Jun 2, 2021

View reviewed changes

update changelog

2ae44df

kristapratico marked this pull request as ready for review June 2, 2021 18:42

kristapratico requested review from iscai-msft, maririos and mshaban-msft June 2, 2021 18:42

pylint

f175eda

kristapratico requested review from danieljurek, mitchdenny, scbedd and weshaggard as code owners June 2, 2021 18:48

kristapratico removed request for danieljurek, mitchdenny, scbedd and weshaggard June 2, 2021 18:49

maririos reviewed Jun 2, 2021

View reviewed changes

some docs updates

6181411

maririos approved these changes Jun 2, 2021

View reviewed changes

iscai-msft reviewed Jun 2, 2021

View reviewed changes

missed an await

b8bc3de

This was referenced Jun 3, 2021

[translation] simplify input to translation #19060

Merged

[translation] renames #19063

Merged

iscai-msft approved these changes Jun 3, 2021

View reviewed changes

kristapratico merged commit 77e30b8 into Azure:master Jun 3, 2021

kristapratico deleted the dt-poller-design branch June 3, 2021 15:26

kristapratico restored the dt-poller-design branch June 3, 2021 15:26

Conversation

kristapratico commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristapratico commented Jun 2, 2021

Uh oh!

azure-pipelines bot commented Jun 2, 2021

Uh oh!

maririos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maririos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kristapratico commented Jun 2, 2021 •

edited

Loading