Skip to content

Conversation

@nicain
Copy link
Contributor

@nicain nicain commented Apr 6, 2023

Description

Include the use of the application default credential to get credentials and project_id to construct the ParticipantsClient

As the sample is currently written, the ParticipantsClient._transport._client is already defined from the ADC. However, the snippet allows the project_id to be passed in, for example from GOOGLE_CLOUD_PROJECT. If the ADC quota_project_id and the project_id differ, this can result in really confusing behavior (and nearly impossible for an end-user to debut), when the two projects are on different tiers eg. Enterprise vs. Free.

Note: This fix does not move import statements into region tags, as is the recommended best-practice. The reason is that this results in linting errors that can not be skipped with in-file directives, because there is more than one sample per file. This scope-of-work is tracked by #9505

@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: dialogflow Issues related to the Dialogflow API. labels Apr 6, 2023
@nicain nicain force-pushed the df_participant_management_cred branch from f17bb4c to 76a7f1c Compare April 6, 2023 00:35
@nicain nicain marked this pull request as ready for review April 6, 2023 00:35
@nicain nicain requested review from a team as code owners April 6, 2023 00:35
Copy link
Member

@rsamborski rsamborski left a comment

Choose a reason for hiding this comment

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

Tests need fixing:

TypeError: analyze_content_audio_stream() got an unexpected keyword argument 'project_id'

And linter is is throwing errors as well:

./participant_management.py:114:1: I100 Import statements are in the wrong order. 'import google.auth' should be before 'from google.cloud import dialogflow_v2beta1'
import google.auth
^
./participant_management.py:114:1: E402 module level import not at top of file
import google.auth
^
./participant_management.py:115:1: F811 redefinition of unused 'dialogflow' from line 19
from google.cloud import dialogflow_v2beta1 as dialogflow
^
./participant_management.py:115:1: E402 module level import not at top of file
from google.cloud import dialogflow_v2beta1 as dialogflow

@m-strzelczyk
Copy link
Contributor

So we lose control over in which project the tests run? They will run in the python-docs-samples-tests no matter what, because the SA that runs them is from that project?

@nicain
Copy link
Contributor Author

nicain commented Apr 6, 2023

So we lose control over in which project the tests run? They will run in the python-docs-samples-tests no matter what, because the SA that runs them is from that project?

It is possible that a user of this sample (in this case me, while debugging a separate problem resolved by #9489) can specify the project ID using GOOGLE_CLOUD_PROJECT, and have the sample use streaminganalyzecontent (SAC) from a different project if the ADC is set using GOOGLE_APPLICATION_CREDENTIALS. In this case (before this change) the ParticipantsClient was set implicitly to use a credentials object for GRPC from the ADC, thus ignoring GOOGLE_CLOUD_PROJECT for the purposes of SAC.

This change makes this behavior explicit, by constructing the credentials object from the ADC and using the corresponding project_id to define participant_path, and thus a matching SAC request.

Its an intersting failure mode; the client.participant_path method was essentially handed two inconsistent configurations: a project_id, and an internally held quota_project_id from its credentials object. It uses the quota_project_id to define billing, and this was quota_project_id was set to an edition (Free Tier) that would not let the sample execute because the sample requires project_id to be configured to Enterprise. This change to the sample requires project_id and quota_project_id to be the same!

@nicain nicain force-pushed the df_participant_management_cred branch from 76a7f1c to 870594b Compare April 6, 2023 16:28
@nicain
Copy link
Contributor Author

nicain commented Apr 6, 2023

Creating future cleanup issue: #9505

@nicain nicain force-pushed the df_participant_management_cred branch from 870594b to 1b5d45d Compare April 6, 2023 16:34
Copy link
Contributor

@engelke engelke left a comment

Choose a reason for hiding this comment

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

LGTM.

@nicain nicain merged commit 28338df into main Apr 6, 2023
@nicain nicain deleted the df_participant_management_cred branch April 6, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: dialogflow Issues related to the Dialogflow API. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants