> 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167615#file2167615line35> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167615#file2167615line49> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167617#file2167617line54> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167617#file2167617line65> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167617#file2167617line94> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167617#file2167617line106> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167617#file2167617line141> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167617#file2167617line223> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167617#file2167617line226> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167618#file2167618line42> > > > > 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) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167618#file2167618line75> > > > > nit: this style is inconsistent. There are other methods shorter than > > these that are not in one line. made style consistent. > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java > > Lines 54-58 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167619#file2167619line54> > > > > nit: incosistent style. I know. It is different from other places. It follows the style in hive-bridge, and make it clear what is the variable name and what is its value. > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java > > Lines 65-66 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167619#file2167619line65> > > > > nit: incosistent style. fixed > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlDataBase.java > > Lines 27-28 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167622#file2167622line27> > > > > remove lombok removed lombok to be consistent with other files under this folder > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModel.java > > Lines 33 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167625#file2167625line33> > > > > nit: incosistent style can you be more specific? > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java > > Lines 33 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167627#file2167627line33> > > > > nit: inconsistent style in this class. can you be more specific? > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java > > Lines 43 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167630#file2167630line43> > > > > nit: inconsistent style can you be more specific? > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java > > Lines 47-50 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167630#file2167630line47> > > > > nit: Some classes have a new line between the methods and others have > > them all compressed. Please choose a consistent style. all files under /model are using the compressed style as they are just getter and setter > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicationPolicy.java > > Lines 40 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167631#file2167631line40> > > > > nit: style for this method is inconsistent now in compressed style. > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > addons/ml-bridge/src/test/resources/users-credentials.properties > > Lines 2-3 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167647#file2167647line2> > > > > Should we be pushing this information to an open source repository? > > Should we push this to a repository at all? it is for testing only. not real credentials. > On Oct. 3, 2019, 12:10 a.m., Austin Nobis wrote: > > pom.xml > > Lines 766 (patched) > > <https://reviews.apache.org/r/71568/diff/1/?file=2167655#file2167655line766> > > > > I would avoid using lombok if it is not already present in the project. I am checking with Atlas team for preference - 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 > > 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/ > > > Testing > ------- > > add integration tests > > > Thanks, > > Na Li > >
