MART typing #29

lumurillo · 2022-12-15T20:35:51Z

What does this PR do?

Add typing annotation to the MART implementation to make the code more readable and robust.

Type of change

Please check all relevant options.

Improvement (non-breaking)
Bug fix (non-breaking)
New feature (non-breaking)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Testing

Please describe the tests that you ran to verify your changes. Consider listing any relevant details of your test configuration.

pytest --cov=mart

Before submitting

The title is self-explanatory and the description concisely explains the PR
My PR does only one thing, instead of bundling different changes together
I list all the breaking changes introduced by this pull request
I have commented my code
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have run pre-commit hooks with pre-commit run -a command without errors

Did you have fun?

Make sure you had fun coding 🙃

dxoigmn

See comments.

mart/attack/projector.py

dxoigmn · 2022-12-17T17:10:15Z

mart/attack/adversary.py

-    def on_run_start(self, adversary, input, target, model, **kwargs):
+    def on_run_start(
+        self,
+        adversary: Callback,


I think we would want to type this as Adversary?

The Adversary class is defined below in this file, so I decided to use its parent class. Other option is to type it as torch.nn.Module which is the other parent class.

I still think it should be Adversary rather than nn.Module. It definitely should not be Callback. What is the problem with using Adversary? Is it because circular imports? Then we should restructure things. (Or make this nn.Module and create a new PR that fixes the circular import issue.)

Created issue #38 to tackle this. This circular issue can be a sign of a possible design problem between the adversaries and callbacks.

mart/attack/adversary.py

dxoigmn · 2022-12-17T17:15:06Z

mart/attack/adversary_in_art.py

-    def __init__(self, target_model, mart_exp_config_yaml, **kwargs):
+    def __init__(self, target_model: torch.nn.Module, mart_exp_config_yaml: str, **kwargs):


I think this class should adopt the types in https://github.com/Trusted-AI/adversarial-robustness-toolbox/blob/987052c405e05d458276299aafc7d47bb584e738/art/estimators/object_detection/object_detector.py#L42

Should we use the ART's ObjectDetector type in the target_model argument?

Yeah I think that is correct. @mzweilin?

Currently, we expect it to be ObjectDetector. But later we may want to use BaseEstimator as we generalize the wrapper for non-object detection models in ART.

mart/attack/adversary_wrapper.py

mart/attack/objective/object_detection.py

mart/attack/perturber/batch.py

mart/attack/projector.py

dxoigmn

See comments.

dxoigmn · 2023-01-03T19:56:41Z

mart/attack/adversary.py

-    def on_run_start(self, adversary, input, target, model, **kwargs):
+    def on_run_start(
+        self,
+        adversary: Callback,


I still think it should be Adversary rather than nn.Module. It definitely should not be Callback. What is the problem with using Adversary? Is it because circular imports? Then we should restructure things. (Or make this nn.Module and create a new PR that fixes the circular import issue.)

dxoigmn · 2023-01-03T19:57:48Z

mart/attack/adversary_in_art.py

-    def __init__(self, target_model, mart_exp_config_yaml, **kwargs):
+    def __init__(self, target_model: torch.nn.Module, mart_exp_config_yaml: str, **kwargs):


Yeah I think that is correct. @mzweilin?

mart/attack/adversary_wrapper.py

dxoigmn · 2023-01-03T20:17:09Z

mart/attack/callbacks/base.py

-    def on_run_start(self, adversary, input, target, model, **kwargs):
+    def on_run_start(
+        self,
+        adversary: torch.nn.Module,


mart/attack/perturber/batch.py

* Removed typing hints to the AdversaryCallbackHookMixin class, since the base class already documents it.

dxoigmn

One comment about using typing.TYPE_CHECKING to guard new imports.

mart/attack/adversary_in_art.py

dxoigmn

LGTM!

lumurillo added the enhancement New feature or request label Dec 15, 2022

lumurillo self-assigned this Dec 15, 2022

lumurillo changed the title ~~Lumurillo/mart typing~~ MART typing Dec 15, 2022

lumurillo requested review from dxoigmn and mzweilin December 15, 2022 20:41

dxoigmn requested changes Dec 17, 2022

View reviewed changes

lumurillo requested a review from dxoigmn January 3, 2023 17:43

dxoigmn requested changes Jan 3, 2023

View reviewed changes

lumurillo requested a review from dxoigmn January 3, 2023 21:52

mzweilin mentioned this pull request Jan 5, 2023

Refactor Adversary interaction with Callbacks #39

Closed

13 tasks

lumurillo added 19 commits January 10, 2023 08:22

Add type annotations to ThreadModel

c719d05

Add type annotations to Projector

b905d3b

Add type annotation to Initializer

67f729e

Add type annotations to GradientModifier

a761073

Add type annotation to Gain

a55e11e

Add type annotation to Adversary

c5d42ad

Add type annotation to NormalizedAdversaryAdapter

79e5b40

Add type annotation to MartToArtAttackAdapter

3451fa0

Add type annotation to perturbers

a109eb5

Add type annotation to Objectives

3a948ab

Add type annotation to Objectives

b79bee2

Add type annotation to Callback

0cd5f0c

Clean adversary file imports

0561b10

Mark as optional some IterativeGenerator init arguments

08b3c97

Changes suggested from PR

0a618c3

Add callable types

094acf7

Add ART dependency to use its typing

74ef738

Change ART type

2bc22f3

Solve circular import issue between Callback and Adversary

2a9c668

* Removed typing hints to the AdversaryCallbackHookMixin class, since the base class already documents it.

lumurillo force-pushed the lumurillo/mart-typing branch from afad315 to 2a9c668 Compare January 10, 2023 16:49

lumurillo mentioned this pull request Jan 10, 2023

Circular import issue with Adversary reference #38

Closed

dxoigmn reviewed Jan 19, 2023

View reviewed changes

mart/attack/adversary_in_art.py Show resolved Hide resolved

dxoigmn self-requested a review February 2, 2023 17:37

dxoigmn approved these changes Feb 2, 2023

View reviewed changes

lumurillo merged commit c1064d8 into main Feb 2, 2023

lumurillo deleted the lumurillo/mart-typing branch February 2, 2023 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MART typing #29

MART typing #29

lumurillo commented Dec 15, 2022

dxoigmn left a comment

dxoigmn Dec 17, 2022

lumurillo Jan 3, 2023

dxoigmn Jan 3, 2023

lumurillo Jan 3, 2023

dxoigmn Dec 17, 2022

lumurillo Jan 3, 2023

dxoigmn Jan 3, 2023

mzweilin Jan 5, 2023

dxoigmn left a comment

dxoigmn Jan 3, 2023

dxoigmn Jan 3, 2023

dxoigmn Jan 3, 2023

dxoigmn left a comment

dxoigmn left a comment

		def __init__(self, target_model, mart_exp_config_yaml, **kwargs):
		def __init__(self, target_model: torch.nn.Module, mart_exp_config_yaml: str, **kwargs):

MART typing #29

MART typing #29

Conversation

lumurillo commented Dec 15, 2022

What does this PR do?

Type of change

Testing

Before submitting

Did you have fun?

dxoigmn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dxoigmn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dxoigmn left a comment

Choose a reason for hiding this comment

dxoigmn left a comment

Choose a reason for hiding this comment