REVAI-3855: Update python sdk to support Super API #105

kirillatrev · 2023-12-20T17:37:45Z

REVAI-3855: Update python sdk to support Super API
https://revinc.atlassian.net/browse/REVAI-3855

https://revinc.atlassian.net/browse/REVAI-3855

kirillatrev · 2023-12-20T20:45:03Z

Tests are coming shortly

k-weng · 2023-12-20T22:33:15Z

src/rev_ai/apiclient.py

+
+        response = self._make_http_request(
+            "GET",
+            urljoin(self.base_url, 'jobs/{}/transcript/summary'.format(id_)),


Any way to extract 'jobs/{}/transcript/summary' to be reusable?

k-weng · 2023-12-20T22:37:07Z

src/rev_ai/models/asynchronous/job.py

+            speakers_count=None,
+            summarization: Summarization = None,
+            translation: Translation = None):


Suggested change

speakers_count=None,

summarization: Summarization = None,

translation: Translation = None):

speakers_count=None,

summarization: Summarization=None,

translation: Translation=None):

Same as bellow:

I follow PEP8, which is commonly accepted standard for python formatting, PyCharm supports PEP 8 and suggests this formatting here. I Used autoformat everywhere where reasonable.

I think it is less matter what is the autoformatting when all the remaining code will be formatted differently.

at the same time, as per PEP8

Don’t use spaces around the = sign when used to indicate a keyword argument, or when used to indicate a default value for an unannotated function parameter: # Correct: def complex(real, imag=0.0): return magic(r=real, i=imag) # Wrong: def complex(real, imag = 0.0): return magic(r = real, i = imag)

Discussed on Zoom. Agreed that formatting of arguments with type annotation is different according to the standard.

k-weng · 2023-12-20T22:38:17Z

src/rev_ai/models/asynchronous/summarization_options.py

+    def __init__(
+            self,
+            prompt: str = None,
+            model: NlpModel = None,
+            formattingType: SummarizationFormattingOptions = None
+    ):


Is this formatting correct?

I used auto formatter as suggested with linter, no complaints.

I follow PEP8, which is commonly accepted standard for python formatting, PyCharm supports PEP 8 and suggests this formatting here. I Used autoformat everywhere where reasonable.

tests/resources/test_mp3.mp3

k-weng · 2023-12-20T22:48:52Z

tests/test_superapi.py

I would split the unit tests up into simpler test cases. They are pretty hefty, each testing three or four response methods along with the request method. I would maybe split each test case into

request_local_file - assert request with proper params

request_source_url - assert request with proper params

get_transcript_summary_text

get_transcript_summary_json

get_transcript_summary_streamed_json

get_translation...
...

This used to be a real integration test before, but as of now I agree that it makes more sense to split those tests. Thanks!

k-weng

Looking good so far - had some comments regarding simplifying the unit tests and the need for the audio files and transcript file since they can just be mocked in code.

In addition, it would nice to add an example to https://github.com/revdotcom/revai-python-sdk/tree/develop/examples and document some usage in the README - like we have for Human Transcription - https://github.com/revdotcom/revai-python-sdk?tab=readme-ov-file#human-transcription

k-weng · 2023-12-20T22:57:57Z

Can you also bump version to 2.19.0 and add an entry to https://github.com/revdotcom/revai-python-sdk/blob/develop/HISTORY.rst

dmtrrk · 2023-12-21T18:21:12Z

src/rev_ai/apiclient.py

@@ -66,7 +70,9 @@ def submit_job_url(
            notification_config=None,
            skip_postprocessing=False,
            remove_atmospherics=False,
-            speakers_count=None):
+            speakers_count=None,


it looks like we are missing to pass these parameters to _create_job_options_payload

remove_atmospherics=False, speakers_count=None,

@amikofalvy do you know whether it is intentional?

@dmtrrk We just haven't supported them in SDKs yet - there's an open PR for it -#104

dmtrrk · 2023-12-21T18:28:48Z

src/rev_ai/apiclient.py

+
+        response = self._make_http_request(
+            "GET",
+            urljoin(self.base_url, 'jobs/{0}/captions/translation/{2}{1}'.format(id_, query, language)),


why don't you pass params as format(id_, language, query)?
it is a bit difficult to read the code with the unnecessary {2}{1}

The idea was to avoid adding changes to format arguments order, but at this point it is better to change it, Thanks!

dmtrrk · 2023-12-21T18:50:14Z

src/rev_ai/models/asynchronous/job.py

+            speakers_count=None,
+            summarization: Summarization = None,
+            translation: Translation = None):


I think it is less matter what is the autoformatting when all the remaining code will be formatted differently.

at the same time, as per PEP8

Don’t use spaces around the = sign when used to indicate a keyword argument, or when used to indicate a default value for an unannotated function parameter: # Correct: def complex(real, imag=0.0): return magic(r=real, i=imag) # Wrong: def complex(real, imag = 0.0): return magic(r = real, i = imag)

dmtrrk · 2023-12-21T18:52:39Z

src/rev_ai/models/asynchronous/summarization_options.py

+from .summarization_job_status import SummarizationJobStatus
+from ..nlp_model import NlpModel
+
+"""Summarization request options."""


I think this should be inside the class

dmtrrk · 2023-12-21T18:56:09Z

src/rev_ai/models/asynchronous/summarization_options.py

+
+
+class SummarizationOptions:
+    def __init__(


based on other files, it looks like the trailing ): should be on the same line with the last argument.

nit, but this is a public code. also, there should be a comment

dmtrrk · 2023-12-21T18:57:20Z

src/rev_ai/models/asynchronous/summarization_options.py

+        return dict_result
+
+
+"""Summarization options."""


not sure what kind of syntax is this. I believe this should be inside the class

dmtrrk · 2023-12-21T19:00:14Z

src/rev_ai/models/asynchronous/summary.py

@@ -0,0 +1,20 @@
+from typing import List, Dict
+
+"""Transcript summary model."""


this should be either before imports, or inside the class

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

so the file might look like this:

"""Module description here""" import xyz class MyClass: """This is my class""" pass class MyClass2: """This is my class 2""" pass

dmtrrk · 2023-12-21T19:00:48Z

src/rev_ai/models/asynchronous/translation_language_options.py

+            model: NlpModel = None,
+            status: SummarizationJobStatus = None,
+            failure: str = None
+


remove line

dmtrrk · 2023-12-21T19:04:14Z

src/rev_ai/models/asynchronous/translation_language_options.py

+    def __init__(
+            self,
+            language: str = None,
+            model: NlpModel = None


not sure whether you discussed it already., I see that some code is written in py2.x style without the type annotations, other - with..

while strict typing is good, wondering how can we be more consistent in the publicly available code

Agreed to encourage incremental improvements going forward.

dmtrrk · 2023-12-21T19:14:38Z

tests/test_async_summarization.py

+            "POST",
+            JOBS_URL,
+            json={
+                'metadata': "python sdk SuperApi test",


not sure SuperApi should be here

kirillatrev · 2023-12-21T19:40:40Z

Examples to follow in another PR once this update is published

k-weng

LGTM - I'll let @dmtrrk provide the final approval

kirillatrev added 16 commits December 20, 2023 11:35

/REVAI-3855: Update python sdk to support Super API

16f225f

https://revinc.atlassian.net/browse/REVAI-3855

1

c3249d5

1

98a877c

1

ec67023

1

3300947

1

19690b1

1

1646d96

1

d38ac85

1

a045d99

1

35f66e3

1

56b6ba1

1

7feea90

1

1a7be10

1

70a92b6

1

9131661

1

7e93e4e

kirillatrev marked this pull request as ready for review December 20, 2023 20:44

kirillatrev requested a review from a team as a code owner December 20, 2023 20:44

kirillatrev requested review from amikofalvy and k-weng December 20, 2023 20:45

kirillatrev added 4 commits December 20, 2023 15:59

Tests

f2c5083

1

82597ba

1

7510072

1

2b4a1bb

k-weng reviewed Dec 20, 2023

View reviewed changes

tests/resources/test_mp3.mp3 Outdated Show resolved Hide resolved

k-weng reviewed Dec 20, 2023

View reviewed changes

k-weng suggested changes Dec 20, 2023

View reviewed changes

kirillatrev added 5 commits December 20, 2023 22:21

Feedback

74b6321

1

d85bbee

1

33388e9

1

3949ca3

1

58b17ef

k-weng requested a review from dmtrrk December 21, 2023 18:16

dmtrrk requested changes Dec 21, 2023

View reviewed changes

1

71776f0

dmtrrk reviewed Dec 21, 2023

View reviewed changes

kirillatrev added 4 commits December 21, 2023 13:17

1

0c87876

1

a9bcfc0

1

701e5ec

1

a9cc05f

kirillatrev added 3 commits December 21, 2023 13:42

1

fba7a79

1

ea8dea8

1

cb259c9

kirillatrev requested review from dmtrrk and k-weng December 21, 2023 19:50

k-weng approved these changes Dec 21, 2023

View reviewed changes

dmtrrk approved these changes Dec 21, 2023

View reviewed changes

kirillatrev merged commit cfac469 into develop Dec 21, 2023
6 checks passed

kirillatrev deleted the feature/REVAI-3855 branch December 21, 2023 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REVAI-3855: Update python sdk to support Super API #105

REVAI-3855: Update python sdk to support Super API #105

kirillatrev commented Dec 20, 2023

kirillatrev commented Dec 20, 2023

k-weng Dec 20, 2023

k-weng Dec 20, 2023

kirillatrev Dec 20, 2023

dmtrrk Dec 21, 2023

kirillatrev Dec 21, 2023

k-weng Dec 20, 2023

kirillatrev Dec 20, 2023

kirillatrev Dec 20, 2023

k-weng Dec 20, 2023

kirillatrev Dec 20, 2023

k-weng left a comment

k-weng commented Dec 20, 2023

dmtrrk Dec 21, 2023

k-weng Dec 21, 2023

dmtrrk Dec 21, 2023

kirillatrev Dec 21, 2023

dmtrrk Dec 21, 2023

dmtrrk Dec 21, 2023

dmtrrk Dec 21, 2023

dmtrrk Dec 21, 2023

dmtrrk Dec 21, 2023

dmtrrk Dec 21, 2023

dmtrrk Dec 21, 2023

kirillatrev Dec 21, 2023

dmtrrk Dec 21, 2023

kirillatrev commented Dec 21, 2023

k-weng left a comment

		@@ -0,0 +1,20 @@
		from typing import List, Dict

		"""Transcript summary model."""

REVAI-3855: Update python sdk to support Super API #105

REVAI-3855: Update python sdk to support Super API #105

Conversation

kirillatrev commented Dec 20, 2023

kirillatrev commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-weng left a comment

Choose a reason for hiding this comment

k-weng commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirillatrev commented Dec 21, 2023

k-weng left a comment

Choose a reason for hiding this comment