Use google.auth.default() in cirq_google.get_engine #4985
Conversation
maffoo
requested review from
wcourtney,
a team,
vtomole,
cduck and
verult
as code owners
| @@ -834,7 +835,7 @@ def get_engine(project_id: Optional[str] = None) -> Engine: | |||
|
|
|||
| Args: | |||
| project_id: If set overrides the project id obtained from the | |||
| environment variable `GOOGLE_CLOUD_PROJECT`. | |||
There was a problem hiding this comment.
I'm overall in favor of this change and think that this is the way we should go about setting the GCP project, but it's worth noting that the GOOGLE_CLOUD_PROJECT env variable is referenced in several places in the Cirq code: https://github.com/quantumlib/Cirq/search?q=GOOGLE_CLOUD_PROJECT. If we're going to change how the project is set (by using gcloud set project), we should be consistent throughout the framework.
There was a problem hiding this comment.
Thanks for that; I'm happy to go through and clean up more of those usages. To be clear, google.auth.default() does seem to respect the GOOGLE_CLOUD_PROJECT env var, so it's fine to set the project via the environment or with gcloud set project. It'd be nice if we can include some boilerplate language about this in one location and reference that elsewhere (at least for code; notebooks should probably be self-contained). Do you have a preference about where to document this?
There was a problem hiding this comment.
SGTM. It may be worth mentioning this in the "concepts" page here, but it looks like it may have been avoided to lean on the "getting started" tutorial to discuss creation.
It looks like validation only happens in one place today and that the other cases are only in documentation and tests, so maybe we should just generalize the docstrings and point them to the shared documentation (referenced above?) about what it means to "set a project". WDYT?
I don't want to spiral your change out of control, so approving now if you'd like to capture this in a followup issue instead.
maffoo
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
the
automerge
CirqBot
removed
the
front_of_queue_automerge
This will pick up a default project set with `gcloud config set project <project_id>` or with a service account referenced by the env var `GOOGLE_APPLICATION_CREDENTIALS`, in addition to the `GOOGLE_CLOUD_PROJECT` environment variable we were checking before. Note that when we call `google.auth.default()` we also get a credential object, which I'm currently discarding. I presume there's a way to pass that in to the `EngineContext` via `service_args`, which would avoid having to authenticate again, but I'm not sure what args are expected there. @wcourtney, maybe you could shed some light here?
This will pick up a default project set with `gcloud config set project <project_id>` or with a service account referenced by the env var `GOOGLE_APPLICATION_CREDENTIALS`, in addition to the `GOOGLE_CLOUD_PROJECT` environment variable we were checking before. Note that when we call `google.auth.default()` we also get a credential object, which I'm currently discarding. I presume there's a way to pass that in to the `EngineContext` via `service_args`, which would avoid having to authenticate again, but I'm not sure what args are expected there. @wcourtney, maybe you could shed some light here?
This will pick up a default project set with
gcloud config set project <project_id>or with a service account referenced by the env varGOOGLE_APPLICATION_CREDENTIALS, in addition to theGOOGLE_CLOUD_PROJECTenvironment variable we were checking before.Note that when we call
google.auth.default()we also get a credential object, which I'm currently discarding. I presume there's a way to pass that in to theEngineContextviaservice_args, which would avoid having to authenticate again, but I'm not sure what args are expected there. @wcourtney, maybe you could shed some light here?