You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemml.apache.org by GitBox <gi...@apache.org> on 2020/06/03 15:46:25 UTC
[GitHub] [systemml] sebwrede opened a new pull request #946: Privacy Runtime Extended
sebwrede opened a new pull request #946:
URL: https://github.com/apache/systemml/pull/946
- Privacy Constraint support for GLM
- Privacy tests for GLM
- Improved exception handling of federated responses
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] sebwrede commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r441459421
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedResponse.java
##########
@@ -94,10 +94,11 @@ public void throwExceptionFromResponse() throws Exception {
* If the map is empty, it means that no privacy constraints were found.
* @param checkedConstraints map of checked constraints from the PrivacyMonitor
*/
- public void setCheckedConstraints(ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints){
+ public void setCheckedConstraints(HashMap<PrivacyLevel,LongAdder> checkedConstraints){
if ( checkedConstraints != null && !checkedConstraints.isEmpty() ){
- this.checkedConstraints = checkedConstraints;
- }
+ this.checkedConstraints = new HashMap<PrivacyLevel, LongAdder>();
+ this.checkedConstraints.putAll(checkedConstraints);
+ }
Review comment:
Now I changed it anyway. The documentation says that it is efficient :smile:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] sebwrede commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r445388137
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedRequest.java
##########
@@ -74,4 +80,16 @@ public int getNumParams() {
public FederatedRequest deepClone() {
return new FederatedRequest(_method, new ArrayList<>(_data));
}
+
+ public void setCheckPrivacy(boolean checkPrivacy){
+ this.checkPrivacy = checkPrivacy;
+ }
+
+ public void setCheckPrivacy(){
+ setCheckPrivacy(DMLScript.CHECK_PRIVACY);
+ }
+
+ public boolean checkPrivacy(){
+ return checkPrivacy;
+ }
Review comment:
The name is phrased like a question: "Should I check privacy? Yes or no."
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] sebwrede commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r441446884
##########
File path: src/main/java/org/apache/sysds/runtime/privacy/PrivacyMonitor.java
##########
@@ -30,23 +30,33 @@
import org.apache.sysds.runtime.privacy.PrivacyConstraint.PrivacyLevel;
public class PrivacyMonitor
-{
- //TODO maybe maintain a log of checked constaints for transfers
- // in order to provide 'privacy explanations' similar to our stats
- private static ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints = new ConcurrentHashMap<PrivacyLevel,LongAdder>();
+{
+ private static HashMap<PrivacyLevel,LongAdder> checkedConstraints;
+
+ static {
+ checkedConstraints = new HashMap<PrivacyLevel,LongAdder>();
+ for ( PrivacyLevel level : PrivacyLevel.values() ){
+ checkedConstraints.put(level, new LongAdder());
+ }
+ }
+
private static boolean checkPrivacy = false;
- public static ConcurrentHashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
+ public static HashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
return checkedConstraints;
}
private static void incrementCheckedConstraints(PrivacyLevel privacyLevel){
- if ( checkPrivacy )
- checkedConstraints.computeIfAbsent(privacyLevel, k -> new LongAdder()).increment();
+ if ( checkPrivacy ){
+ if ( privacyLevel == null )
+ throw new NullPointerException("Cannot increment checked constraints log: Privacy level is null.");
+ checkedConstraints.get(privacyLevel).increment();
+ }
+
}
public static void clearCheckedConstraints(){
- checkedConstraints.clear();
+ checkedConstraints.replaceAll((k,v)->new LongAdder());
Review comment:
But I only add new key types very rarely. It is the PrivacyLevel enum, which only has three different types right now. When it comes to the code, the current implementation would not require me to change anything in this map in PrivacyMonitor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] sebwrede commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r441442854
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedResponse.java
##########
@@ -94,10 +94,11 @@ public void throwExceptionFromResponse() throws Exception {
* If the map is empty, it means that no privacy constraints were found.
* @param checkedConstraints map of checked constraints from the PrivacyMonitor
*/
- public void setCheckedConstraints(ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints){
+ public void setCheckedConstraints(HashMap<PrivacyLevel,LongAdder> checkedConstraints){
if ( checkedConstraints != null && !checkedConstraints.isEmpty() ){
- this.checkedConstraints = checkedConstraints;
- }
+ this.checkedConstraints = new HashMap<PrivacyLevel, LongAdder>();
+ this.checkedConstraints.putAll(checkedConstraints);
+ }
Review comment:
It is not necessary for it to be concurrent. An enum should have the same hash value and it is unlikely to conflict with other enums. I could also have used EnumMap, but it is not necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] Baunsgaard commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r441439819
##########
File path: src/main/java/org/apache/sysds/runtime/privacy/PrivacyMonitor.java
##########
@@ -30,23 +30,33 @@
import org.apache.sysds.runtime.privacy.PrivacyConstraint.PrivacyLevel;
public class PrivacyMonitor
-{
- //TODO maybe maintain a log of checked constaints for transfers
- // in order to provide 'privacy explanations' similar to our stats
- private static ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints = new ConcurrentHashMap<PrivacyLevel,LongAdder>();
+{
+ private static HashMap<PrivacyLevel,LongAdder> checkedConstraints;
+
+ static {
+ checkedConstraints = new HashMap<PrivacyLevel,LongAdder>();
+ for ( PrivacyLevel level : PrivacyLevel.values() ){
+ checkedConstraints.put(level, new LongAdder());
+ }
+ }
+
private static boolean checkPrivacy = false;
- public static ConcurrentHashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
+ public static HashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
return checkedConstraints;
}
private static void incrementCheckedConstraints(PrivacyLevel privacyLevel){
- if ( checkPrivacy )
- checkedConstraints.computeIfAbsent(privacyLevel, k -> new LongAdder()).increment();
+ if ( checkPrivacy ){
+ if ( privacyLevel == null )
+ throw new NullPointerException("Cannot increment checked constraints log: Privacy level is null.");
+ checkedConstraints.get(privacyLevel).increment();
+ }
+
}
public static void clearCheckedConstraints(){
- checkedConstraints.clear();
+ checkedConstraints.replaceAll((k,v)->new LongAdder());
Review comment:
Once you add a new key value pair, you have to add it two places. Both in the clearCheckedContraints and in the initial key-value pairs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] Baunsgaard commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r441449635
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedResponse.java
##########
@@ -94,10 +94,11 @@ public void throwExceptionFromResponse() throws Exception {
* If the map is empty, it means that no privacy constraints were found.
* @param checkedConstraints map of checked constraints from the PrivacyMonitor
*/
- public void setCheckedConstraints(ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints){
+ public void setCheckedConstraints(HashMap<PrivacyLevel,LongAdder> checkedConstraints){
if ( checkedConstraints != null && !checkedConstraints.isEmpty() ){
- this.checkedConstraints = checkedConstraints;
- }
+ this.checkedConstraints = new HashMap<PrivacyLevel, LongAdder>();
+ this.checkedConstraints.putAll(checkedConstraints);
+ }
Review comment:
EnumMap sounds good :) did not know that one, but as you say it is not necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] kev-inn commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
kev-inn commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r445086179
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedRequest.java
##########
@@ -74,4 +80,16 @@ public int getNumParams() {
public FederatedRequest deepClone() {
return new FederatedRequest(_method, new ArrayList<>(_data));
}
+
+ public void setCheckPrivacy(boolean checkPrivacy){
+ this.checkPrivacy = checkPrivacy;
+ }
+
+ public void setCheckPrivacy(){
+ setCheckPrivacy(DMLScript.CHECK_PRIVACY);
+ }
+
+ public boolean checkPrivacy(){
+ return checkPrivacy;
+ }
Review comment:
would `getCheckPrivacy` maybe be more consistent with the `set`? Would be fine with both, just throwing it out there.
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedResponse.java
##########
@@ -56,10 +65,45 @@ public boolean isSuccessful() {
}
public String getErrorMessage() {
- return (String) _data[0];
+ return ExceptionUtils.getFullStackTrace( (Exception) _data[0] );
}
- public Object[] getData() {
+ public Object[] getData() throws Exception {
+ updateCheckedConstraintsLog();
+ if ( !isSuccessful() )
+ throwExceptionFromResponse();
Review comment:
Big fan of this :+1:
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedRequest.java
##########
@@ -33,20 +35,24 @@
private FedMethod _method;
private List<Object> _data;
+ private boolean checkPrivacy;
Review comment:
could we prepend it with `_` so the variables have the same format?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] Baunsgaard commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r441375740
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedResponse.java
##########
@@ -94,10 +94,11 @@ public void throwExceptionFromResponse() throws Exception {
* If the map is empty, it means that no privacy constraints were found.
* @param checkedConstraints map of checked constraints from the PrivacyMonitor
*/
- public void setCheckedConstraints(ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints){
+ public void setCheckedConstraints(HashMap<PrivacyLevel,LongAdder> checkedConstraints){
if ( checkedConstraints != null && !checkedConstraints.isEmpty() ){
- this.checkedConstraints = checkedConstraints;
- }
+ this.checkedConstraints = new HashMap<PrivacyLevel, LongAdder>();
+ this.checkedConstraints.putAll(checkedConstraints);
+ }
Review comment:
question.
Are you leveraging that it is not concurrent? or that the likelihood of two elements having the same hash is low?
##########
File path: src/main/java/org/apache/sysds/runtime/privacy/PrivacyMonitor.java
##########
@@ -30,23 +30,33 @@
import org.apache.sysds.runtime.privacy.PrivacyConstraint.PrivacyLevel;
public class PrivacyMonitor
-{
- //TODO maybe maintain a log of checked constaints for transfers
- // in order to provide 'privacy explanations' similar to our stats
- private static ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints = new ConcurrentHashMap<PrivacyLevel,LongAdder>();
+{
+ private static HashMap<PrivacyLevel,LongAdder> checkedConstraints;
+
+ static {
+ checkedConstraints = new HashMap<PrivacyLevel,LongAdder>();
+ for ( PrivacyLevel level : PrivacyLevel.values() ){
+ checkedConstraints.put(level, new LongAdder());
+ }
+ }
+
private static boolean checkPrivacy = false;
- public static ConcurrentHashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
+ public static HashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
return checkedConstraints;
}
private static void incrementCheckedConstraints(PrivacyLevel privacyLevel){
- if ( checkPrivacy )
- checkedConstraints.computeIfAbsent(privacyLevel, k -> new LongAdder()).increment();
+ if ( checkPrivacy ){
+ if ( privacyLevel == null )
+ throw new NullPointerException("Cannot increment checked constraints log: Privacy level is null.");
+ checkedConstraints.get(privacyLevel).increment();
+ }
+
}
public static void clearCheckedConstraints(){
- checkedConstraints.clear();
+ checkedConstraints.replaceAll((k,v)->new LongAdder());
Review comment:
once you add more functions, this will become an duplication from lines ~40.
Maybe make a builderPattern and use that in both cases?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] sebwrede commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r441438074
##########
File path: src/main/java/org/apache/sysds/runtime/privacy/PrivacyMonitor.java
##########
@@ -30,23 +30,33 @@
import org.apache.sysds.runtime.privacy.PrivacyConstraint.PrivacyLevel;
public class PrivacyMonitor
-{
- //TODO maybe maintain a log of checked constaints for transfers
- // in order to provide 'privacy explanations' similar to our stats
- private static ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints = new ConcurrentHashMap<PrivacyLevel,LongAdder>();
+{
+ private static HashMap<PrivacyLevel,LongAdder> checkedConstraints;
+
+ static {
+ checkedConstraints = new HashMap<PrivacyLevel,LongAdder>();
+ for ( PrivacyLevel level : PrivacyLevel.values() ){
+ checkedConstraints.put(level, new LongAdder());
+ }
+ }
+
private static boolean checkPrivacy = false;
- public static ConcurrentHashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
+ public static HashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
return checkedConstraints;
}
private static void incrementCheckedConstraints(PrivacyLevel privacyLevel){
- if ( checkPrivacy )
- checkedConstraints.computeIfAbsent(privacyLevel, k -> new LongAdder()).increment();
+ if ( checkPrivacy ){
+ if ( privacyLevel == null )
+ throw new NullPointerException("Cannot increment checked constraints log: Privacy level is null.");
+ checkedConstraints.get(privacyLevel).increment();
+ }
+
}
public static void clearCheckedConstraints(){
- checkedConstraints.clear();
+ checkedConstraints.replaceAll((k,v)->new LongAdder());
Review comment:
What is being duplicated?
One of them creates the initial key-value pairs the other one resets them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] kev-inn commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
kev-inn commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r445399020
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedRequest.java
##########
@@ -74,4 +80,16 @@ public int getNumParams() {
public FederatedRequest deepClone() {
return new FederatedRequest(_method, new ArrayList<>(_data));
}
+
+ public void setCheckPrivacy(boolean checkPrivacy){
+ this.checkPrivacy = checkPrivacy;
+ }
+
+ public void setCheckPrivacy(){
+ setCheckPrivacy(DMLScript.CHECK_PRIVACY);
+ }
+
+ public boolean checkPrivacy(){
+ return checkPrivacy;
+ }
Review comment:
If you view it this way I agree that seems more readable :+1:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemml] sebwrede commented on a change in pull request #946: Privacy Runtime Extended
Posted by GitBox <gi...@apache.org>.
sebwrede commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r445387201
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedRequest.java
##########
@@ -33,20 +35,24 @@
private FedMethod _method;
private List<Object> _data;
+ private boolean checkPrivacy;
Review comment:
Good idea!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org