Dataproc API doesn't return imageUri in a consistent format [139762442]

Fixed

Bug

Status Update

No update yet.

Description

de...@cleardata.com

created issue #1

Aug 20, 2019 10:07PM

The Dataproc API returns different formats for the imageUri field, depending on how the user supplied it. If the caller supplies a full resource name for the image, imageUri will contain the full resource name. But if it the caller supplies a relative resource name, imageUri returns the relative resource name.

Stock images always seem to return the full resource name. The API should always normalize imageUri so apps can depend on the format being a full resource name.

Steps to reproduce:

1) Create two clusters via the API, one with a full name, the other with a relative name for the custom image. (I'm using python with the discovery API here):

>>> from googleapiclienthelpers.discovery import build_subresource
>>> dataproc = build_subresource('dataproc.projects.regions.clusters', 'v1beta2')
>>> dataproc.create(projectId='cleardata-lab-krv', region='us-central1', body={'clusterName': 'api-relative-name', 'config': {'masterConfig': {'imageUri': 'projects/cleardata-images/global/images/cleardataproc-debian9-1-4-10-20190804'}}}).execute()
{'name': 'projects/cleardata-lab-krv/regions/us-central1/operations/31d2b055-c099-4c58-9126-93dd05562df0',
'metadata': {'@type': '

type.googleapis.com/google.cloud.dataproc.v1beta2.ClusterOperationMetadata',
'clusterName': 'api-relative-name',
'clusterUuid': 'b0e9ebf2-e385-41a3-acef-2b5b5ff16b12',
'status': {'state': 'PENDING',
'innerState': 'PENDING',
'stateStartTime': '2019-08-20T21:55:57.801Z'},
'operationType': 'CREATE',
'description': 'Create cluster with 2 workers',
'warnings': ['For PD-Standard without local SSDs, we strongly recommend provisioning 1TB or larger to ensure consistently high I/O performance. See

https://cloud.google.com/compute/docs/disks/performance for information on disk I/O performance.']}}
>>> dataproc.create(projectId='cleardata-lab-krv', region='us-central1', body={'clusterName': 'api-full-name', 'config': {'masterConfig': {'imageUri': '

https://www.googleapis.com/compute/v1/projects/cleardata-images/global/images/cleardataproc-debian9-1-4-10-20190804'}}}).execute()
{'name': 'projects/cleardata-lab-krv/regions/us-central1/operations/95890614-cf09-4d03-ab3c-55a6fea36a1c',
'metadata': {'@type': '

type.googleapis.com/google.cloud.dataproc.v1beta2.ClusterOperationMetadata',
'clusterName': 'api-full-name',
'clusterUuid': '7f0a6cb3-87fd-4a1c-a39b-b8af4b060dcf',
'status': {'state': 'PENDING',
'innerState': 'PENDING',
'stateStartTime': '2019-08-20T21:56:23.205Z'},
'operationType': 'CREATE',
'description': 'Create cluster with 2 workers',
'warnings': ['For PD-Standard without local SSDs, we strongly recommend provisioning 1TB or larger to ensure consistently high I/O performance. See

https://cloud.google.com/compute/docs/disks/performance for information on disk I/O performance.']}}

2) wait for all clusters to complete creating.

3) Describe them and note the different formats for imageUri:
$ gcloud dataproc clusters describe api-relative-name --region=us-central1 | grep imageUri
imageUri: projects/cleardata-images/global/images/cleardataproc-debian9-1-4-10-20190804
imageUri: projects/cleardata-images/global/images/cleardataproc-debian9-1-4-10-20190804
$ gcloud dataproc clusters describe api-full-name --region=us-central1 | grep imageUri
imageUri:

https://www.googleapis.com/compute/v1/projects/cleardata-images/global/images/cleardataproc-debian9-1-4-10-20190804
imageUri:

https://www.googleapis.com/compute/v1/projects/cleardata-images/global/images/cleardataproc-debian9-1-4-10-20190804

Thanks,
Ross

Comments

ab...@google.com <ab...@google.com> Jan 13, 2020 08:50PM

Assigned to gc...@google.com.

ab...@google.com <ab...@google.com> #2Jan 20, 2020 06:49PM

One design decision worth elaboring on is whether or not to rename InstIntrinsicsCall to InstIntrinsics. With InstIntrinsicsCall no longer derived from InstCall, to get rid of the Target parameter, it can never get lowered into a call (at least not a call for which the target is unknown to the backend). In high-level languages, intrinsics look like function calls, but at Subzero's IR level they look more like instructions. Hence dropping the "Call" from the name would make sense. Alternatives discussed with Antonio include:

Keep the InstCall target operand separate from the Srcs operand list, so the goal of aligning the source operands of load/store-like intrinsics with those of regular load/store instructions is still achieved. Unfortunately this causes complications for liveness analysis, since the target operand of a regular call can be a function pointer produced by other instructions which need to be considered live. We could teach Subzero to treat the target operand as an additional source operand, but it's a bit risky and might require more than liveness analysis to be updated.
Move the target operand to the last position of Srcs[]. This works without affecting things like liveness analysis, but is still a bit risky since we must ensure nothing looks for the target operand at index 0. It would also still be an unused undefined value for intrinsics.
Add virtual methods for accessing load/store operands. Currently there are no virtual methods for Inst, and since we process many thousands of these it could have a performance impact to make virtual calls. Also note it already uses a Kind enum to perform its own RTTI.

It's worth noting that to LLVM, intrinsics are definitely considered functions: https://llvm.org/docs/LangRef.html#intrinsic-functions. They can even only be invoked through a call instruction. Despite Subzero's design clearly having been inspired by LLVM, it uses a separate class for representing intrinsics. InstIntrinsicCall also takes an Intrinsics::IntrinsicInfo argument at construction, whereas with LLVM that information is part of the call target.

Hence I intend on moving forward with renaming it to InstIntrinsics, and adding some comments to clarify that it represents a extension instruction which few parts of the compiler have to know the exact functionality about.

mk...@google.com <mk...@google.com> #3Jan 23, 2020 11:47PM

Project: SwiftShader
Branch: master

commit 33a77f7f129311dcbb7e1480af812de2fe9177db
Author: Nicolas Capens <capn@google.com>
Date: Mon Feb 08 15:04:38 2021

Rename InstIntrinsicCall to InstIntrinsic

It is no longer derived from InstCall, and doesn't take a Target
parameter which could be a symbol to a function which implements
the intrinsic.

Note one can still emit actual Call instructions if a function call
is needed. Since the previous change which removed the Target parameter
we can no longer decide between implementing an intrinsic as inline
instructions or a function call at the Subzero level, but we can still
do that at the Reactor level which has its own concept of intrinsics.

This change also removes mentions of intrinsics representing function
calls. It also removes code related to PNaCl-specific LLVM intrinsics,
including the ability to look up intrinsics by name. The addArg(),
getArg(), and getNumArgs() methods, adopted from InstCall (but no longer
inherited from it), are kept for now due to risk of replacing the ones
for InstCall objects, while the confusion caused by keeping the
function-related "arg" term is deemed low.

Bug:

b/179497998

Change-Id: I293f039853abff6f5bebda1b714774205bdec846
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52608
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M src/Reactor/Optimizer.cpp
M src/Reactor/SubzeroReactor.cpp
M third_party/subzero/docs/DESIGN.rst
M third_party/subzero/src/IceCfg.cpp
M third_party/subzero/src/IceConverter.cpp
M third_party/subzero/src/IceELFObjectWriter.cpp
M third_party/subzero/src/IceGlobalContext.cpp
M third_party/subzero/src/IceGlobalContext.h
M third_party/subzero/src/IceGlobalInits.cpp
M third_party/subzero/src/IceGlobalInits.h
M third_party/subzero/src/IceInst.cpp
M third_party/subzero/src/IceInst.h
M third_party/subzero/src/IceInstrumentation.cpp
M third_party/subzero/src/IceInstrumentation.h
M third_party/subzero/src/IceIntrinsics.cpp
M third_party/subzero/src/IceIntrinsics.h
M third_party/subzero/src/IceTargetLowering.cpp
M third_party/subzero/src/IceTargetLowering.h
M third_party/subzero/src/IceTargetLoweringARM32.cpp
M third_party/subzero/src/IceTargetLoweringARM32.h
M third_party/subzero/src/IceTargetLoweringMIPS32.cpp
M third_party/subzero/src/IceTargetLoweringMIPS32.h
M third_party/subzero/src/IceTargetLoweringX86Base.h
M third_party/subzero/src/IceTargetLoweringX86BaseImpl.h
M third_party/subzero/src/PNaClTranslator.cpp
M third_party/subzero/src/WasmTranslator.cpp

https://swiftshader-review.googlesource.com/52608

di...@google.com <di...@google.com> #4Feb 13, 2020 02:27AM

Project: SwiftShader
Branch: master

commit 673a7fe5c3ff6686c98951cafb24a93ca5bd1303
Author: Nicolas Capens <capn@google.com>
Date: Fri Feb 05 15:03:22 2021

Unify load/store operand accessors

Load and store instructions, as well as intrinsics which access memory,
can now shared the same methods for accessing the memory address and
data operands.

Note that while this change introduces the potential for non-load/store
instructions to have their operands accessed through getLoadAddress(),
getStoreAddress(), or getData(), that risk isn't any greater than using
the wrong getSrc() index, and would stick out as a mistake much more
clearly. The advantage this change brings is that we no longer have to
remember where the address and data operands are stored in sub-vector
load/store intrinsics. In addition, there are no more overly verbose
casts, and their cost is eliminated.

Bug:

b/179497998

Change-Id: I0d9208555e00b0d3053f7d3baca241fef2b8cbeb
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52531
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M src/Reactor/Optimizer.cpp
M third_party/subzero/src/IceInst.h
M third_party/subzero/src/IceTargetLowering.cpp
M third_party/subzero/src/IceTargetLoweringARM32.cpp
M third_party/subzero/src/IceTargetLoweringMIPS32.cpp
M third_party/subzero/src/IceTargetLoweringX86BaseImpl.h

https://swiftshader-review.googlesource.com/52531

de...@cleardata.com <de...@cleardata.com> #5Feb 13, 2020 05:14PM

Project: SwiftShader
Branch: master

commit 8d50b556a5decb8a256c196282e42060a806fd35
Author: Nicolas Capens <capn@google.com>
Date: Fri Feb 05 14:55:30 2021

Discern between load and store addresses

There were InstLoad::getSourceAddress() and InstStore::getAddr()
methods, which aren't very clear and consistently named. This change
replaces them with getLoadAddress() and getStoreAddress(), respectively.

This will also enable moving these methods to the Inst class to make
them available for SubVectorLoad and SubVectorStore intrinsics. While
these methods don't make sense for other instructions, note that
Inst::getSrc() already provides access to all operands and has to be
used with knowledge of the operand meaning and layout. So this only
provides a name to these operands, and it would stick out as a sore
thumb if used incorrectly.

Bug:

b/179497998

Change-Id: I86b1201b8a1c611682f4f91541bdb49e17ef71a8
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52530
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M src/Reactor/Optimizer.cpp
M third_party/subzero/src/IceASanInstrumentation.cpp
M third_party/subzero/src/IceInst.cpp
M third_party/subzero/src/IceInst.h
M third_party/subzero/src/IceTargetLoweringARM32.cpp
M third_party/subzero/src/IceTargetLoweringMIPS32.cpp
M third_party/subzero/src/IceTargetLoweringX86BaseImpl.h

https://swiftshader-review.googlesource.com/52530

mk...@google.com <mk...@google.com> #6Jun 19, 2021 12:41AM

Marked as fixed.

Project: SwiftShader
Branch: master

commit 99bbb14b1b98a467521552003cd0f5604186cbd4
Author: Nicolas Capens <capn@google.com>
Date: Fri Feb 05 14:13:37 2021

Eliminate the InstIntrinsicCall Target parameter

It is no longer used now that profiling support at the Subzero level
is eliminated.

This change adjusts all of the uses of getSrc() on intrinsics to obtain
the correct operand, but does not yet make simplifications based on
having them align with load/store instructions.

Bug:

b/179497998

Change-Id: I93705eaa1b7626184f612ab3a9755048004e531f
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52529
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M src/Reactor/Optimizer.cpp
M src/Reactor/SubzeroReactor.cpp
M third_party/subzero/src/IceInst.h
M third_party/subzero/src/IceTargetLoweringX86BaseImpl.h

https://swiftshader-review.googlesource.com/52529

Issue 139762442

Description

Issue summary

Comments

ab...@google.com <ab...@google.com> Jan 13, 2020 08:50PM

ab...@google.com <ab...@google.com> #2Jan 20, 2020 06:49PM

mk...@google.com <mk...@google.com> #3Jan 23, 2020 11:47PM

di...@google.com <di...@google.com> #4Feb 13, 2020 02:27AM

de...@cleardata.com <de...@cleardata.com> #5Feb 13, 2020 05:14PM

mk...@google.com <mk...@google.com> #6Jun 19, 2021 12:41AM

Add comment

Issue metadata