subject:"Review Request 71568\: ATLAS\-3432\: Add Machine Learning Governance integration"

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-07 Thread Karthik Manamcheri via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218124
---

addons/models/1000-Hadoop/1110-ml_model.json
Lines 1 (patched)

I think this should follow the conventions stated in this blog post

https://blog.cloudera.com/creating-an-open-standard-machine-learning-governance-using-apache-atlas/

Basically strip out the idea of "ml_model" and directly go from project to 
model build.

Here is my implementation which is based on the standards proposed in the 
blog post
https://reviews.apache.org/r/71567/diff/1#21

The reason I am pushing for the standard is because it becomes difficult to 
update schemas (especially removing types).

addons/models/1000-Hadoop/1110-ml_model.json
Lines 15 (patched)

All types should have a guid or uuid. We can call it "resourceName" or 
"resourceIdentified" or "qualifiedName" or whatever. But there should be an 
identifier associated which is unique and identifiable. 

For example, this could be arn (amazon resource name) or a crn (cloudera 
resource name)

addons/models/1000-Hadoop/1110-ml_model.json
Lines 165 (patched)

Anything related to kubernetes needs to be optional. We are designing a 
generic type system and this might not be a kubernetes deployement.

addons/models/1000-Hadoop/1110-ml_model.json
Lines 182 (patched)

Why is cpuMillicores optional and the other memory related things not 
optional? I think this should be optional as well.

addons/models/1000-Hadoop/1110-ml_model.json
Lines 204 (patched)

We should remove this type completely (as per a previous comment) on 
removing things related to training until we have a good understanding of how 
to attach training data.

addons/models/1000-Hadoop/1110-ml_model.json
Lines 284 (patched)

I think this should be an optional field.

addons/models/1000-Hadoop/1110-ml_model.json
Lines 318 (patched)

What if the build is not an s2i build? This should be optional. 

Actually, most of the fields should be optional except for:

1. user name
2. createdAt (timestamps)
3. uniqueName

addons/models/1000-Hadoop/1110-ml_model.json
Lines 420 (patched)

Do we need this relationship definition? The model and model deployment are 
related through lineage, no? You can follow the lineage from model to model 
deployment. Do we need another layer of relationship duplicated?

addons/models/1000-Hadoop/1110-ml_model.json
Lines 439 (patched)

Same question as before. Why duplicate this relationship when the lineage 
can define the project -> model relationship.

- Karthik Manamcheri

On Oct. 4, 2019, 8:21 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71568/
> ---
> 
> (Updated Oct. 4, 2019, 8:21 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/README.md PRE-CREATION 
>   addons/ml-bridge/pom.xml PRE-CREATION 
>   
> addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/HookToolConfiguration.java
>  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
>

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-07 Thread Austin Nobis via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218119
---




addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 145-169 (patched)


I would recommend at least switching to wget instead of using curl if we 
are going to kick the can on switching this to an HTTP client. Curl is not 
installed by default and could/will cause issues. I know this was an issue with 
Impala relying on curl.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 67 (patched)


I think the comment needs to be removed if this is reolved or an issue 
opened for the default.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 110 (patched)


Does some clean up need to occur in the 'finally' block? If not can we 
remove it?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 123-127 (patched)


It appears nothing is happening here. Can we remove this code?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 137 (patched)


Do we want an OR here instead of an AND?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 181 (patched)


hit: s/unknown/not supported/



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java
Lines 80 (patched)


nit: because the variable name is too long it is not aligned with 
everything else.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java
Lines 203 (patched)


nit: what is this spacing?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java
Lines 221-222 (patched)


nit: what is this spacing?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java
Lines 249 (patched)


nit: what is this spacing?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java
Lines 272 (patched)


nit: what is this spacing?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModel.java
Lines 54-66 (patched)


nit: should these methods be one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModel.java
Lines 73-79 (patched)


nit: Should these 2 methods also be one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java
Lines 59-61 (patched)


nit: should this be one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java
Lines 67-69 (patched)


nit: should this be one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java
Lines 73-75 (patched)


nit: should this be one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java
Lines 85-87 (patched)


nit: should this be one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java
Lines 89-91 (patched)


nit: should this be one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java
Lines 99-101 (patched)


nit: should this be one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java
Lines 43-45 (patched)


nit: Should this be on one line?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicationPolicy.java
Lines 35 (patched)


nit: spacing after 'int' is not

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-07 Thread Karthik Manamcheri via Review Board



> 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)
> > 
> >
> > "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?
> 
> Na Li wrote:
> This is not specific for CDSW. It contains just full name and json string.

In that case, rename it to "MachineLearningEntity". I want to avoid the term 
"Entity" which could cause confusion with the Atlas term Entity.


- Karthik


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218027
---


On Oct. 4, 2019, 8:21 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71568/
> ---
> 
> (Updated Oct. 4, 2019, 8:21 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/README.md PRE-CREATION 
>   addons/ml-bridge/pom.xml PRE-CREATION 
>   
> addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/HookToolConfiguration.java
>  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/mlModelDeployments.json PRE-CREATION 
>   addons/ml-bridge/src/test/resources/mlModelDeploymentsTrainingData.json 
> PRE-CREATION 
>   addons/ml-bridge/src/test/resources/mlModels.json PRE-CREATION 
>   addons/ml-bridge/src/test/resources/mlModelsTrainingData.json PRE-CREATION 
>

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-04 Thread Na Li via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/
---

(Updated Oct. 4, 2019, 8:21 p.m.)


Review request for atlas, Austin Nobis, Ashutosh Mestry, and Sarath Subramanian.


Repository: atlas


Description
---

add integration for ML


Diffs (updated)
-

  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/README.md PRE-CREATION 
  addons/ml-bridge/pom.xml PRE-CREATION 
  
addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/HookToolConfiguration.java
 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/mlModelDeployments.json PRE-CREATION 
  addons/ml-bridge/src/test/resources/mlModelDeploymentsTrainingData.json 
PRE-CREATION 
  addons/ml-bridge/src/test/resources/mlModels.json PRE-CREATION 
  addons/ml-bridge/src/test/resources/mlModelsTrainingData.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/3/

Changes: https://reviews.apache.org/r/71568/diff/2-3/


Testing
---

add integration tests


Thanks,

Na Li

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-04 Thread Na Li via Review Board



> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
> > Lines 35 (patched)
> > 
> >
> > How do we know the class name but need to load it via reflection? Where 
> > is this class defined and how is the JAR going to be included with Atlas at 
> > runtime?

the ml-bridge-shim module and ml-bridge module are implemented together. That 
is why we know the class name. The deployment will make sure the real implement 
jar will be put at the right folder. This is pattern in Atlas. It is to make 
sure the caller of the shim jar (which does not have real implementation, and 
only depends on Atlas classloader) won't load many Atlas classes and therefore 
causes issue for its class path.


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
> > Lines 49 (patched)
> > 
> >
> > Is this style common in Atlas? I've seen it in Hive not sure about 
> > Atlas. If it is common why is it only present in this file and nowhere else?

I followed the style in hive-bridge. This style is used in functions that are 
entry point of processing, and makes the message stands out.


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
> > Lines 54-60 (patched)
> > 
> >
> > I think all of these could be placed into a Configuration class as 
> > opposed to bloating this class with configuration and functionality.

good point. done.


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
> > Lines 65 (patched)
> > 
> >
> > I think it is strange making references to closed source software 
> > (CDSW) in open source.

agree. it is replaced with general term.


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
> > Lines 94 (patched)
> > 
> >
> > I think it could cause issues if we return here instead of exiting with 
> > a non-zero exit code.

done.


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
> > Lines 106 (patched)
> > 
> >
> > I don't think this comment is useful. Add javadoc for this method if it 
> > is needed.

changed to javadoc


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
> > Lines 141 (patched)
> > 
> >
> > Remove comment or add java doc.

done


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
> > Lines 223 (patched)
> > 
> >
> > curl isn't installed on all machines by default. I think we should use 
> > the Java HTTP client to do this instead of spawning a separate process and 
> > using curl.

created "ATLAS-3440 Get ML metadata from ML service using HTTP client" to track 
this


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
> > Lines 226 (patched)
> > 
> >
> > Are we sending the unencrypted username and password? I don't think 
> > that is very secure. Is there no other way to do this communication?

this class is only used for testing. It should not be used for production. In 
production, the ML service will call the hook implementation to pass in the ML 
metadata.


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java
> > Lines 42 (patched)
> > 
> >
> > nit: why is there a big space after this.hook?

removed extra white space


> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java
> > Lines 75 (patched)
> > 
> >

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-04 Thread Na Li via Review Board



> On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote:
> > pom.xml
> > Lines 766 (patched)
> > 
> >
> > I would avoid using lombok if it is not already present in the project.
> 
> Na Li wrote:
> I am checking with Atlas team for preference

removed


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218032
---


On Oct. 2, 2019, 11:31 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71568/
> ---
> 
> (Updated Oct. 2, 2019, 11:31 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/README.md 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 
>   
>

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-04 Thread Na Li via Review Board



> On Oct. 3, 2019, 4:55 a.m., radford nguyen wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
> > Lines 228 (patched)
> > 
> >
> > Apache commons has a built-in helper for printing usage text.  If you 
> > use it, then you don't have to duplicate the option descriptions in 
> > `printUsage`:
> > 
> > ```java
> > private static void printUsage(Options options) {
> > HelpFormatter formatter = new HelpFormatter();
> > formatter.printHelp("ml_hook.sh ", options);
> > }
> > ```
> > 
> > As an added bonus, if you ever change any options, you don't have to 
> > change `printUsage`

thanks for mentioning this. code is updated.


> On Oct. 3, 2019, 4:55 a.m., radford nguyen wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java
> > Lines 32 (patched)
> > 
> >
> > `hook` doesn't seem to be used in this class. Should it be removed?

done


> On Oct. 3, 2019, 4:55 a.m., radford nguyen wrote:
> > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
> > Lines 125 (patched)
> > 
> >
> > This is a general commentary, but I am of the opinion that 
> > `AtlasMlGovernanceProcessContext` should be immutable, since it is passed 
> > around to many different consumers, and you generally want all of them to 
> > receive the exact same data (namely that which identifies the context).  
> > With a mutable object, the data can change at any point underneath you and 
> > you wouldn't know it, which opens up the possibility of bugs that would be 
> > hard to trace, especially in a multi-threaded environment.
> > 
> > With `AtlasMlGovernanceProcessContext` immutable, we would then 
> > refactor this method slightly to have the signature:
> > ```java
> > protected AtlasMlGovernanceProcessContext 
> > parseHookContext(HookContext context)
> > ```
> > The method signature itself would then identify the purpose of the 
> > method, which is to take a `HookContext` as _input_ and return a 
> > `AtlasMlGovernanceProcessContext` as _output_.

changed the signature.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218040
---


On Oct. 2, 2019, 11:31 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71568/
> ---
> 
> (Updated Oct. 2, 2019, 11:31 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/README.md 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 
>   
>

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-04 Thread Na Li via Review Board



> On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote:
> > addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
> > Lines 28 (patched)
> > 
> >
> > This comment is confusing. What is the ML Governance server? I think 
> > that's an internal CDSW detail.
> 
> Na Li wrote:
> Does this comment works for you "This class is used to convert machine 
> learning metadata to lineage notifications
>  and send them to Atlas."?
>  
>  If not, what's your suggestion?
> 
> Karthik Manamcheri wrote:
> That works. Mention CDSW as well since this understands only CDSW-style 
> metadata. There is no universal machine learning metadata.
> 
> "This class is used to convert machine learning metadata from CDSW into 
> lineage notifications"

The feedback from Atlas team is to not mention CDSW.


> 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)
> > 
> >
> > 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.
> 
> Na Li wrote:
> 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.

Based on feedback from Atlas team, we should not mention CDSW


> On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote:
> > addons/models/1000-Hadoop/1110-ml_model.json
> > Lines 11 (patched)
> > 
> >
> > What is "serviceType" used for? In the future, if I want to extend this 
> > system for non-cdsw use-cases should I change this?

“serviceType” is a grouping of related entity typedefs e.g.“hive”, “impala”. I 
changed the service type to "ml". for other use-cases, we can use a different 
name.


> On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote:
> > addons/models/1000-Hadoop/1110-ml_model.json
> > Lines 204 (patched)
> > 
> >
> > 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.

I removed the way to get training data.


> On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote:
> > pom.xml
> > Lines 1574 (patched)
> > 
> >
> > Same goes here. This should be cdsw-bridge.

the hook API is general. should not be CDSW specific.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218027
---


On Oct. 2, 2019, 11:31 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71568/
> ---
> 
> (Updated Oct. 2, 2019, 11:31 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/README.md 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

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-02 Thread radford nguyen


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218040
---




addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 228 (patched)


Apache commons has a built-in helper for printing usage text.  If you use 
it, then you don't have to duplicate the option descriptions in `printUsage`:

```java
private static void printUsage(Options options) {
HelpFormatter formatter = new HelpFormatter();
formatter.printHelp("ml_hook.sh ", options);
}
```

As an added bonus, if you ever change any options, you don't have to change 
`printUsage`



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java
Lines 32 (patched)


`hook` doesn't seem to be used in this class. Should it be removed?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 125 (patched)


This is a general commentary, but I am of the opinion that 
`AtlasMlGovernanceProcessContext` should be immutable, since it is passed 
around to many different consumers, and you generally want all of them to 
receive the exact same data (namely that which identifies the context).  With a 
mutable object, the data can change at any point underneath you and you 
wouldn't know it, which opens up the possibility of bugs that would be hard to 
trace, especially in a multi-threaded environment.

With `AtlasMlGovernanceProcessContext` immutable, we would then refactor 
this method slightly to have the signature:
```java
protected AtlasMlGovernanceProcessContext parseHookContext(HookContext 
context)
```
The method signature itself would then identify the purpose of the method, 
which is to take a `HookContext` as _input_ and return a 
`AtlasMlGovernanceProcessContext` as _output_.


- radford nguyen


On Oct. 2, 2019, 11:31 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71568/
> ---
> 
> (Updated Oct. 2, 2019, 11:31 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/README.md 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 
>   
>

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-02 Thread Austin Nobis via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218032
---



Some initial comments. I will make more comments later.


addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 35 (patched)


How do we know the class name but need to load it via reflection? Where is 
this class defined and how is the JAR going to be included with Atlas at 
runtime?



addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 49 (patched)


Is this style common in Atlas? I've seen it in Hive not sure about Atlas. 
If it is common why is it only present in this file and nowhere else?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 54-60 (patched)


I think all of these could be placed into a Configuration class as opposed 
to bloating this class with configuration and functionality.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 65 (patched)


I think it is strange making references to closed source software (CDSW) in 
open source.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 94 (patched)


I think it could cause issues if we return here instead of exiting with a 
non-zero exit code.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 106 (patched)


I don't think this comment is useful. Add javadoc for this method if it is 
needed.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 141 (patched)


Remove comment or add java doc.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 223 (patched)


curl isn't installed on all machines by default. I think we should use the 
Java HTTP client to do this instead of spawning a separate process and using 
curl.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java
Lines 226 (patched)


Are we sending the unencrypted username and password? I don't think that is 
very secure. Is there no other way to do this communication?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java
Lines 42 (patched)


nit: why is there a big space after this.hook?



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java
Lines 75 (patched)


nit: this style is inconsistent. There are other methods shorter than these 
that are not in one line.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 54-58 (patched)


nit: incosistent style.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
Lines 65-66 (patched)


nit: incosistent style.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlDataBase.java
Lines 27-28 (patched)


remove lombok



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModel.java
Lines 33 (patched)


nit: incosistent style



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java
Lines 33 (patched)


nit: inconsistent style in this class.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java
Lines 43 (patched)


nit: inconsistent style



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java
Lines 47-50 (patched)


nit: Some classes have a new line between the methods and others have them 
all compressed. Please choose a consistent style.



addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicationPolicy.java
Lines 40 (patched)

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-02 Thread Na Li via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/
---

(Updated Oct. 2, 2019, 11:31 p.m.)


Review request for atlas, Austin Nobis, Ashutosh Mestry, and Sarath Subramanian.


Repository: atlas


Description
---

add integration for ML


Diffs (updated)
-

  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/README.md 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/2/

Changes: https://reviews.apache.org/r/71568/diff/1-2/


Testing
---

add integration tests


Thanks,

Na Li

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-02 Thread Na Li via Review Board



> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > "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)
> > 
> >
> > 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

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-02 Thread Karthik Manamcheri via Review Board



> On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote:
> > addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
> > Lines 28 (patched)
> > 
> >
> > This comment is confusing. What is the ML Governance server? I think 
> > that's an internal CDSW detail.
> 
> Na Li wrote:
> Does this comment works for you "This class is used to convert machine 
> learning metadata to lineage notifications
>  and send them to Atlas."?
>  
>  If not, what's your suggestion?

That works. Mention CDSW as well since this understands only CDSW-style 
metadata. There is no universal machine learning metadata.

"This class is used to convert machine learning metadata from CDSW into lineage 
notifications"


- Karthik


---
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 
>

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-02 Thread Na Li via Review Board



> On Oct. 2, 2019, 7:56 p.m., Karthik Manamcheri wrote:
> > addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java
> > Lines 28 (patched)
> > 
> >
> > This comment is confusing. What is the ML Governance server? I think 
> > that's an internal CDSW detail.

Does this comment works for you "This class is used to convert machine learning 
metadata to lineage notifications
 and send them to Atlas."?
 
 If not, what's your suggestion?


- 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 
>   
>

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-02 Thread Karthik Manamcheri via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/#review218031
---



Add a README file which explains what this is and how to run the hook tool to 
consume CDSW content into Atlas.

- 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 
>

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-02 Thread Karthik Manamcheri via Review Board

---
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)

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)

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)

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)

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)

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)

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)

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)

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)

"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)

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)

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)

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)

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 
>

Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

2019-10-01 Thread Na Li via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71568/
---

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

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Re: Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

Review Request 71568: ATLAS-3432: Add Machine Learning Governance integration

17 matches

Site Navigation

Mail list logo

Footer information