> On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java > > Lines 51 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167617#file2167617line51> > > > > Where or why do we need this? and how do we use this. Please add > > comments here.
added comments > On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java > > Lines 48 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167619#file2167619line48> > > > > The "ML Governance server" is an internal detail of CDSW. We shouldn't > > add comments here about that. This should CDSWLineageHook. In short term, it is. in long term, no > On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java > > Lines 160 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167619#file2167619line160> > > > > For all TODOs create JIRA tickets and attach them here. ATLAS-3435 is created > On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java > > Lines 444 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167620#file2167620line444> > > > > I don't think we should provide hacky implementations. Remove > > everything to do with training_data. removed the hacky approach > On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlLifecycleInfo.java > > Lines 33 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167624#file2167624line33> > > > > 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. the fields are in json string. Info from other sources can be formated to a json string with those fields. That is why I named the classes with more general names, not CDSW specific. > On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > addons/ml-bridge/src/main/resources/atlas-log4j.xml > > Lines 33 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167637#file2167637line33> > > > > This will log ONLY from the class "MlGovernanceLineageHook". You should > > define a root logger to get logs out of the other classes as well. I believe the behavior is that for GovernanceLineageHook, the log level is "info". For other classes, the log level is warning by default.every class can log messages. I changed the logger name to cover all atlas classes > On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > addons/ml-bridge/src/test/java/org/apache/atlas/ml/governance/GovernanceHookIT.java > > Lines 35 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167639#file2167639line35> > > > > What is "IT". Expand it. This is "IntegrationTest", right? It is for integration test. Using "IT" is Atlas convention. > On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/Entity.java > > Lines 28 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167649#file2167649line28> > > > > "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? This is not specific for CDSW. It contains just full name and json string. > On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote: > > pom.xml > > Lines 1568 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167655#file2167655line1568> > > > > 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. The hook is very general. Its data structure is not CDSW specific. Maybe we can call the hook implementation CDSW specific. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71568/#review218027 ----------------------------------------------------------- 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 > >
