feat: MLmodel Graphql Query #2166
Conversation
|
@jjoyce0510 : could you take a look at this? |
arunvasudevan
force-pushed
the
mlmodel_graphql_query
branch
from
5fc4f9b to
7a3cc71
Compare
|
|
||
| ``` | ||
| { | ||
| mlmodel(urn: "urn:li:mlModel:(urn:li:dataPlatform:science,scienceModel,PROD)") { |
There was a problem hiding this comment.
nit: Can we name this Query "mlModel"?
| } | ||
| } | ||
| } | ||
| mlModelProperties { |
There was a problem hiding this comment.
nit: Can we rename this to "properties"?
| primaryUsers | ||
| outOfScopeUses | ||
| } | ||
| mlModelFactorPrompts { |
There was a problem hiding this comment.
nit: can we rename this to factorPrompts?
| @@ -91,6 +93,17 @@ public static DataPlatforms getDataPlatformsClient() { | |||
| return _dataPlatforms; | |||
| } | |||
|
|
|||
| public static MLModels getMlModelsClient() { | |||
| @@ -91,6 +93,17 @@ public static DataPlatforms getDataPlatformsClient() { | |||
| return _dataPlatforms; | |||
| } | |||
|
|
|||
| public static MLModels getMlModelsClient() { | |||
| if (_mlmodels == null) { | |||
| } else if (env.getObject() instanceof FloatBox) { | ||
| return env.getSchema().getObjectType(FLOAT_BOX); | ||
| } else { | ||
| throw new RuntimeException("Unrecognized object type provided to type resolver"); |
There was a problem hiding this comment.
Let's include the env.getObject() string in the error message for help debugging
|
|
||
| @Override | ||
| public GraphQLObjectType getType(TypeResolutionEnvironment env) { | ||
| if (env.getObject() instanceof StringBox) { |
There was a problem hiding this comment.
As of now its just String - https://github.com/linkedin/datahub/blob/master/metadata-models/src/main/pegasus/com/linkedin/ml/metadata/ResultsType.pdl
It's made a Union type to expand in the future
...l-core/src/main/java/com/linkedin/datahub/graphql/types/common/mappers/AuditStampMapper.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public Cost apply(com.linkedin.common.Cost cost) { |
There was a problem hiding this comment.
Is cost common? If not, we can create a new ML models-specific directory to contain these mappers
There was a problem hiding this comment.
Yes, Cost is part of common. Internally we added Cost as an Aspect to Dataset too.
|
|
||
| @Override | ||
| public CostValue apply(com.linkedin.common.CostValue costValue) { | ||
| CostValue result = new CostValue(); |
...-core/src/main/java/com/linkedin/datahub/graphql/types/mappers/DatasetUpdateInputMapper.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| if (datasetUpdateInput.getDeprecation() != null) { | ||
| final DatasetDeprecation deprecation = new DatasetDeprecation(); | ||
| deprecation.setDeprecated(datasetUpdateInput.getDeprecation().getDeprecated()); | ||
| deprecation.setDecommissionTime(datasetUpdateInput.getDeprecation().getDecommissionTime()); | ||
| if (datasetUpdateInput.getDeprecation().getDecommissionTime() != null) { |
There was a problem hiding this comment.
Is this null check required? If decommission time = null, then we will just set the value of DatasetDeprecation.decomissionTime to null also
There was a problem hiding this comment.
In Case of Mutations if the user does not provide decommisionTime in the input then trying to read it would throw an NPE.
It would work in case of queries but for mutations null checks needs to be made for Optional fields.
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/types/mlmodel/MLModelType.java
Show resolved
Hide resolved
| * Maps Pegasus {@link RecordTemplate} objects to objects conforming to the GQL schema. | ||
| * | ||
| */ | ||
| public class MlModelMapper implements ModelMapper<com.linkedin.ml.MLModel, MLModel> { |
There was a problem hiding this comment.
naming nit: Let's name "MLModelMapper" to keep the capitalization consistent across model & code
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/types/mlmodel/MLModelType.java
Show resolved
Hide resolved
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/types/mlmodel/MLModelType.java
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public BrowseResults browse(@Nonnull List<String> path, | ||
| @Nullable List<FacetFilterInput> filters, |
| @Nullable List<FacetFilterInput> filters, | ||
| int start, | ||
| int count, | ||
| @Nonnull final QueryContext context) throws Exception { |
There was a problem hiding this comment.
Do you plan to implement browse in the client within this pr? If not I'd suggest making this a SearchableEntityType only, until that functionality gets in
|
|
||
| import com.linkedin.common.urn.MLModelUrn; | ||
|
|
||
| public class MlModelUtils { |
There was a problem hiding this comment.
"MLModelUtils" to be consistent
| public CaveatDetails apply(@NonNull final com.linkedin.ml.metadata.CaveatDetails input) { | ||
| final CaveatDetails result = new CaveatDetails(); | ||
| if (input.getCaveatDescription() != null) { | ||
| result.setCaveatDescription(input.getCaveatDescription()); |
There was a problem hiding this comment.
Are these null checks required? These can be skipped if there's no risk of a NullPointerException being thrown
There was a problem hiding this comment.
(& if the destination field is optional in the GQL schema)
|
|
||
| @Override | ||
| public EthicalConsiderations apply(@NonNull final com.linkedin.ml.metadata.EthicalConsiderations ethicalConsiderations) { | ||
| EthicalConsiderations result = new EthicalConsiderations(); |
...n/java/com/linkedin/datahub/graphql/types/mlmodel/mappers/HyperParameterValueTypeMapper.java
Show resolved
Hide resolved
...n/java/com/linkedin/datahub/graphql/types/mlmodel/mappers/HyperParameterValueTypeMapper.java
Show resolved
Hide resolved
| @Override | ||
| public MLModelProperties apply(@NonNull final com.linkedin.ml.metadata.MLModelProperties mlModelProperties) { | ||
| final MLModelProperties result = new MLModelProperties(); | ||
| if (mlModelProperties.getDate() != null) { |
There was a problem hiding this comment.
Are all of these null checks required?
There was a problem hiding this comment.
Removed it for Date, Description and Type for other Objects throws NPE
|
|
||
| @Override | ||
| public MLModel apply(@Nonnull final com.linkedin.ml.MLModel mlModel) { | ||
| MLModel result = new MLModel(); |
| result.setType(EntityType.MLMODEL); | ||
| result.setName(mlModel.getName()); | ||
| result.setDescription(mlModel.getDescription()); | ||
| result.setOrigin(Enum.valueOf(FabricType.class, mlModel.getOrigin().name())); |
There was a problem hiding this comment.
You should be able to use something like:
FabricType.valueOf(ml.getOrigin().getName())
here
| result.setMetrics(MetricsMapper.map(mlModel.getMetrics())); | ||
| } | ||
| if (mlModel.getEvaluationData() != null) { | ||
| result.setEvaluationData(mlModel.getEvaluationData().getEvaluationData().stream().map(BaseDataMapper::map).collect(Collectors.toList())); |
There was a problem hiding this comment.
both getEvaluationData() and getEvaluationData().getEvaluationData() will not return null, right? Otherwise risk of NPE
There was a problem hiding this comment.
getEvaluationData().getEvaluationData() does not return null it is a mandatory field - https://github.com/linkedin/datahub/blob/master/metadata-models/src/main/pegasus/com/linkedin/ml/metadata/EvaluationData.pdl#L13
| result.setEvaluationData(mlModel.getEvaluationData().getEvaluationData().stream().map(BaseDataMapper::map).collect(Collectors.toList())); | ||
| } | ||
| if (mlModel.getTrainingData() != null) { | ||
| result.setTrainingData(mlModel.getTrainingData().getTrainingData().stream().map(BaseDataMapper::map).collect(Collectors.toList())); |
There was a problem hiding this comment.
same comment as above
| Details of Data Proprocessing | ||
| """ | ||
| preProcessing: [String] |
| Measures of MLModel performance | ||
| """ | ||
| performanceMeasures: [String] |
| Decision Thresholds used (if any)? | ||
| """ | ||
| decisionThreshold: [String] |
| Primary Use cases for the model. | ||
| """ | ||
| primaryUses: [String] |
| Out of scope uses of the MLModel | ||
| """ | ||
| outOfScopeUses: [String] |
| Primary Intended Users | ||
| """ | ||
| primaryUsers: [IntendedUserType] |
|
|
||
| enum IntendedUserType { | ||
|
|
There was a problem hiding this comment.
Any descriptions available for these?
| Source Code along with types | ||
| """ | ||
| sourceCode: [SourceCodeUrl] |
There was a problem hiding this comment.
[SourceCodeUrl!] - --- Should this field be optional?
There was a problem hiding this comment.
No, its not. Added !
| } | ||
|
|
||
| type SourceCodeUrl { |
There was a problem hiding this comment.
I do not think these should be optional. Use !
|
|
||
| enum SourceCodeUrlType { | ||
|
|
There was a problem hiding this comment.
Any descriptions available?
| } | ||
|
|
||
| type Cost { |
There was a problem hiding this comment.
I don't think these should be optional. Use !
| } | ||
|
|
||
| type CostValue { |
There was a problem hiding this comment.
CostValue is either an ID or a Code. So this is optional
| type Deprecation { | ||
| """ | ||
| Whether the dataset has been deprecated by owner |
There was a problem hiding this comment.
Dataset? We should replace this with "Whether the entity has been deprecated by owner"
There was a problem hiding this comment.
Same for comments below :)
arunvasudevan
force-pushed
the
mlmodel_graphql_query
branch
from
7a3cc71 to
796f761
Compare
|
Thanks for taking time to review @jjoyce0510. |
This PR includes changes in
datahub-graphql-coreto include GraphQL Query functionality changes for MLModel Entity.Includes fixes in GMS based on Tests for MLModel
Also organized mappers into packages by entity.
Checklist