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.

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

adds python-v3 files #156

Merged
merged 8 commits into from
Nov 14, 2023
Merged

adds python-v3 files #156

merged 8 commits into from
Nov 14, 2023

Conversation

SandraRodgers
Copy link
Contributor

@SandraRodgers SandraRodgers commented Oct 23, 2023

This PR adds the bulk of the work that has been done so far for the Python-SDK-v3:

  • Entry point in the deepgram package is init.py, which contains the create_client function
  • deepgram_client is the main class for interacting with the API. It returns the ListenClient, ManageClient, or OnPremClient
  • clients folder holds the various clients
  • types folder is broken up into types categories rather than having one types file for all the types in the project
  • examples folder contains three demos to show the prerecorded, live, and management clients working. This folder will be updated later to follow the format of other examples folders in other SDKs projects

Copy link
Contributor

@davidvonthenen davidvonthenen left a comment

Choose a reason for hiding this comment

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

Overall, very good! 👏 I left some comments most of which aren't code related. Just project management-ish related.

Only other thing would be to have examples for each "thing" (billing, project, keys, etc) where each example shows all of the billing CRUD operations. so demo_billing.py, demo_keys.py, etc. This could be done as a subsequent PR, but good to have with the code that changes it.

.env Outdated Show resolved Hide resolved
deepgram/clients/__pycache__/__init__.cpython-310.pyc Outdated Show resolved Hide resolved
deepgram/clients/abstract_restful_client.py Outdated Show resolved Hide resolved
deepgram/types/__pycache__/_types.cpython-310.pyc Outdated Show resolved Hide resolved
examples/prerecorded/main.py Outdated Show resolved Hide resolved
davidvonthenen
davidvonthenen previously approved these changes Oct 27, 2023
Copy link
Contributor

@davidvonthenen davidvonthenen left a comment

Choose a reason for hiding this comment

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

LGTM

Anything that gets shaken out afterwards, we can always do in a subsequent PR.

examples/demo_prerecorded.py Show resolved Hide resolved
examples/demo_manage.py Outdated Show resolved Hide resolved
deepgram/clients/listen_client.py Outdated Show resolved Hide resolved
deepgram/clients/prerecorded_client.py Outdated Show resolved Hide resolved
.env_example Outdated Show resolved Hide resolved
@jkroll-deepgram
Copy link
Contributor

Are there plans for a top-level readme update? Also for getting an updated structure in place for testing? Or will these come in later PRs?

@jjmaldonis
Copy link
Contributor

I'm viewing the code in the branch rather than looking at the PR changes, so I'll make a big comment with my thoughts here :)

All of these comments are purely preference, so feel free to ignore any/all.

  1. +1 to Julia's idea of using the Black formatter. I use it on all my projects and it keeps the code formatted in the same way for everyone.
  2. In the Deepgram SDK's constructor, Deepgram(api_key, options), I'd prefer to see the options as keyword arguments. This allows my IDE to immediately help me identify which parameters I can pass in. Alternatively, the option params could be documented in the docstring, but I personally don't like that because docstrings are rarely read and can easily forget to be updated (by me!). ... I see an options dictionary get used primarily when backwards compatibility is an issue. Frankly, we aren't very careful about maintaining backwards compatibility in the SDKs and I don't think we should be (so good on us!). The vast majority of changes we'll likely making after this v3 API change will be additions that should not break backwards compat. ... So with both of those points in mind, I think it'd be very nice to have a normal constructor with all the kwargs written out rather than embedded in a DeepgramClientOptions obejct.
    a. I just looked at the DeepgramClientOptions object and how it is used in the Deepgram.__init__ method. I don't think it's clear how the headers and url can be modified by the user. It took me about 5 min to figure it out after carefully reading the code and docstrings, so I think we can significantly simplify the code and the docs by adding the headers and url as kwargs. If we really don't want to do that, then I think the 2nd best option is to use .set methods, e.g. deepgram.set_headers and deepgram.set_url.
  3. Can we use dataclasses rather than TypedDicts? The syntax is very similar but the functionality is much better. If we use dataclasses, we can decorate them with dataclasses_json to make it easy to convert to/from dictionaries/json.
  4. I think the is_json method in helpers.py should be removed and the calling-code should be updated to use try/except. When is_json returns True, the json data gets deserialized twice: once by is_json and a 2nd time by the calling code. JSON deserialization is quite slow, so only running it once should make a noticeable difference (although I guess the HTTP requests will probably always by 100x slower, so ... maybe ignore ... but it's likely to be a criticism of anyone reading the code).
  5. I would vote to make the on-prem client a subclient of the management client. The on-prem client looks like it performs credential management, which imo makes it part of the management API. When I saw DeepgramClient.onprem, I assumed I should use that class to make calls to the /listen API when I'm using an onprem instance, which is incorrect. Maybe some docstrings on the .onprem, .manage, and .listen properties would help, but my vote is still for making the on-prem client a subclient of the manage client.
  6. Can we entirely remove the types in types/prerecorded_types.py? I'm guessing these are intended to be used for validation, but I hope we can find a different way to validate the inputs. tbh I don't want to have to write deepgram.listen.prerecorded.transcribe_url(source={"url": "https://..."}). The {"url": "https://..."} seems unnecessary when I've already specified prerecorded.transcribe_url.
    a. One of Python's core idealogies is called duck typing: “If it walks like a duck, and it quacks like a duck, then it must be a duck.” This philosophy means that if I pass an object into a method, the method should just try to use the object no matter what it is. It's okay if the method performs validation before using it, but I shouldn't need to be extremely explicit about the thing I'm passing in. By passing it in, I'm saying "assume this thing is a duck".
  7. If a callback is passed to prerecorded.transcribe_url, can we automatically call prerecorded.transcribe_url_callback rather than throw an error? I think we know what the caller is intending to do, so let's do it. Same for prerecorded.transcribe_file.
  8. I'd love to see the live.on method be broken up into multiple methods: one for each LiveTranscriptionEvents. It would be nicer to call live.on_transcript(print) than live.on(LiveTranscriptionEvents.Transcript, print) because in the former I don't need to look up LiveTranscriptionEvents.
  9. It looks like some of the complex error handling that was implemented in v2 for the prerecorded and live clients is not implemented in v3? Is that still on the todo list or is it being removed? (or am I totally missing it?)

@SandraRodgers
Copy link
Contributor Author

Are there plans for a top-level readme update? Also for getting an updated structure in place for testing? Or will these come in later PRs?

Yes, I'll add the readme update in another PR

@jpvajda
Copy link
Contributor

jpvajda commented Oct 31, 2023

@jjmaldonis Thank you for the detailed feedback. Could you give the team the sense of the priorities for each one of these suggestions so we can better understand how to prioritize this feedback?

For example it be helpful to see something like:

1 - HIGH
2 - LOW
.. etc.

@jjmaldonis
Copy link
Contributor

No problem.

  1. High bc I think it's very easy to do and adds significant value, but it's not user-facing
    (run pip install black -> cd .../deepgram-python-sdk -> black . -> git commit -u . -> git commit -m "ran black formatter" -> git push and you're done! 3 minutes)
  2. High bc it's a major breaking change
  3. High bc it's a major breaking change
  4. Low bc it can be done later
  5. Medium/Low, depending on other people's opinions on this. It's a breaking change.
  6. High bc it's a major breaking change
  7. Low bc it can be done later without a breaking change
  8. Low bc it can be done later without a breaking change
  9. High bc it seems like a major regression in functionality and I expect users to be unhappy about needing to implement complex error handling after upgrading from v2 to v3

@jjmaldonis
Copy link
Contributor

(I really wish GH let us reply to comments!)

I realized I didn't say this, but take any of those recommendations with a grain of salt. If you don't want to do them, then no worries.

I can also help out with some of them if you don't have time but want the changes made.

@jpvajda
Copy link
Contributor

jpvajda commented Nov 1, 2023

Thanks @jjmaldonis. @dvonthenen is taking the lead on the Python SDK development to finish it up, so I'll let him consider this feedback and decide which approach to take.

@SandraRodgers SandraRodgers deleted the sr/restructure-files branch November 14, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants