----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71568/#review218027 -----------------------------------------------------------
addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java Lines 28 (patched) <https://reviews.apache.org/r/71568/#comment305492> This comment is confusing. What is the ML Governance server? I think that's an internal CDSW detail. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java Lines 51 (patched) <https://reviews.apache.org/r/71568/#comment305493> Where or why do we need this? and how do we use this. Please add comments here. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java Lines 48 (patched) <https://reviews.apache.org/r/71568/#comment305495> The "ML Governance server" is an internal detail of CDSW. We shouldn't add comments here about that. This should CDSWLineageHook. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java Lines 160 (patched) <https://reviews.apache.org/r/71568/#comment305496> For all TODOs create JIRA tickets and attach them here. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java Lines 444 (patched) <https://reviews.apache.org/r/71568/#comment305497> I don't think we should provide hacky implementations. Remove everything to do with training_data. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlLifecycleInfo.java Lines 33 (patched) <https://reviews.apache.org/r/71568/#comment305498> A general comment applicable to your naming convention. Any object which represents a CDSW object should be prefixed with CDSW_something. For example, this representation of "lifecycle" is very CDSW specific and we should call it that. For exmaple, CdswLifecycleInfo This comment applies to all the objects which you are parsing out of CDSW Json. addons/ml-bridge/src/main/resources/atlas-log4j.xml Lines 33 (patched) <https://reviews.apache.org/r/71568/#comment305500> This will log ONLY from the class "MlGovernanceLineageHook". You should define a root logger to get logs out of the other classes as well. addons/ml-bridge/src/test/java/org/apache/atlas/ml/governance/GovernanceHookIT.java Lines 35 (patched) <https://reviews.apache.org/r/71568/#comment305501> What is "IT". Expand it. This is "IntegrationTest", right? addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/Entity.java Lines 28 (patched) <https://reviews.apache.org/r/71568/#comment305502> "Entity" has a specific meaning in Atlas. I think we should rename this. Maybe to "CdswObject" or something. As I understand, these are very specific CDSW defined objects, no? addons/models/1000-Hadoop/1110-ml_model.json Lines 11 (patched) <https://reviews.apache.org/r/71568/#comment305504> What is "serviceType" used for? In the future, if I want to extend this system for non-cdsw use-cases should I change this? addons/models/1000-Hadoop/1110-ml_model.json Lines 204 (patched) <https://reviews.apache.org/r/71568/#comment305505> I don't think we should have training data represented here. We don't have a way to pull training data from CDSW. Let's not overdefine without implementation. pom.xml Lines 1568 (patched) <https://reviews.apache.org/r/71568/#comment305490> I don't think we should call this "ml-hook-api". This is specifically targeting one product (CDSW) and so we should call this the cdsw-hook-api. pom.xml Lines 1574 (patched) <https://reviews.apache.org/r/71568/#comment305491> Same goes here. This should be cdsw-bridge. - Karthik Manamcheri On Oct. 1, 2019, 10:10 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71568/ > ----------------------------------------------------------- > > (Updated Oct. 1, 2019, 10:10 p.m.) > > > Review request for atlas, Austin Nobis, Ashutosh Mestry, and Sarath > Subramanian. > > > Repository: atlas > > > Description > ------- > > add integration for ML > > > Diffs > ----- > > addons/hive-bridge/pom.xml 13384cb > addons/ml-bridge-shim/pom.xml PRE-CREATION > > addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java > PRE-CREATION > addons/ml-bridge/pom.xml PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/CreateMlGovernanceLineage.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlDataBase.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlDataType.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlLifecycleInfo.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModel.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelBuild.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelRef.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlProject.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicationPolicy.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicationStatus.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlResources.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlTrainingData.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlTrainingDataEntry.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlUser.java > PRE-CREATION > addons/ml-bridge/src/main/resources/atlas-log4j.xml PRE-CREATION > addons/ml-bridge/src/main/resources/governance_hook.sh PRE-CREATION > > addons/ml-bridge/src/test/java/org/apache/atlas/ml/governance/GovernanceHookIT.java > PRE-CREATION > > addons/ml-bridge/src/test/java/org/apache/atlas/ml/governance/GovernanceHookITBase.java > PRE-CREATION > addons/ml-bridge/src/test/resources/atlas-application.properties > PRE-CREATION > addons/ml-bridge/src/test/resources/atlas-log4j.xml PRE-CREATION > addons/ml-bridge/src/test/resources/cdswModelDeployments.json PRE-CREATION > addons/ml-bridge/src/test/resources/cdswModelDeploymentsTrainingData.json > PRE-CREATION > addons/ml-bridge/src/test/resources/cdswModels.json PRE-CREATION > addons/ml-bridge/src/test/resources/cdswModelsTrainingData.json > PRE-CREATION > addons/ml-bridge/src/test/resources/users-credentials.properties > PRE-CREATION > addons/ml-hook-api/pom.xml PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/Entity.java > PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/EntityType.java > PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/GovernanceHook.java > PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/HookContext.java > PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/MethodType.java > PRE-CREATION > addons/models/1000-Hadoop/1110-ml_model.json PRE-CREATION > pom.xml ce7d6fb > > > Diff: https://reviews.apache.org/r/71568/diff/1/ > > > Testing > ------- > > add integration tests > > > Thanks, > > Na Li > >
