-
Notifications
You must be signed in to change notification settings - Fork 1
Better support for target models on the ensemble attack #135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 44 commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
1a40fd7
wip
lotif 1d18580
wip
lotif e42e630
WIP moving forward with the ensemble attack code changes
lotif a46a010
WIP adding training and sythesizing code
lotif 30c0ed3
More info on readme
lotif 9464962
More ctgan changes
lotif e5c8fda
Adding the split data code
lotif 8f10678
More config changes and bug fixes
lotif 077d909
Removing ids dynamically
lotif b711fbd
Working!
lotif efdde68
Merge branch 'main' into marcelo/ensamble-ctgan
lotif 1a38af2
Fixing indent on config file and adding some more information to the …
lotif af4f04e
Adding test attack model code
lotif 5afb774
Small bug fixes
lotif e4ec793
Updates to readme and config file values
lotif 1c13126
Small changes on configs and script bug fixes
lotif 4e9a8c9
Adding the compute attack success script and fixing minor issues
lotif d83aabf
Cr by CodeRabbit and Sara
lotif a198fe9
Reducing the amount of training samples to 20k
lotif 0416dbc
Merge branch 'main' into marcelo/ensamble-ctgan
lotif e69b07e
Change function name to avoid pytest thinking it's a test
lotif 579d0f3
Merge remote-tracking branch 'origin/marcelo/ensamble-ctgan' into mar…
lotif 5fa4fef
Fixing test assertions
lotif 8b6bf10
Merge branch 'main' into marcelo/ensamble-ctgan
lotif a9369f6
Making population_all_with_challenge.csv into a constant and adding a…
lotif 163bba8
Addressing last comments by Fatemeh
lotif bf805c1
Merge branch 'main' into marcelo/ensamble-ctgan
lotif ecab1e2
WIP adding model runner class
lotif dda8c5e
Merge branch 'main' into marcelo/support-attack-models
lotif 38a20b5
working first refactor
lotif ac1a0bf
train attack model working
lotif 2c3fa1e
Adding changes for the test model script
lotif ca87ac3
Linter changes
lotif cfb4ded
Merge branch 'main' into marcelo/support-attack-models
lotif c42ee6e
Fixing mypy and ruff
lotif 093b0e4
Tests passing
lotif d50ff39
renaming model to models
lotif 7135924
Small bug fix
lotif 082ea7c
Bringing back the config json saving function against my will
lotif 94da62e
one more bug fix
lotif cc2cb81
Fixing the test
lotif fe78e34
One more refactor to make things simpler.
lotif 26f88f6
CR by Coderabbit
lotif 5137a87
Fixing a bug on the amount of shadow model samples to generate
lotif 068d936
CR by David
lotif e0007e8
Merge branch 'main' into marcelo/support-attack-models
emersodb 4d9b325
Merge branch 'main' into marcelo/support-attack-models
emersodb ad047ce
CR by David and Sara
lotif 4d0a048
Test fixes
lotif File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you've already thought of this, but should the code above be part of the base for the ModelRunner? That is, should lines 87-94 actually happen inside that class rather than in the attack script here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also slightly simplify the process of subbing out the model, since you would just need to sub the runner class instead of both the running and the config class? I might be missing a complexity though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understood your idea, but I thought maybe if I pass the config dictionary to the init of the model runner class we would be able to skip making the config. Is that it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of. My thought was that you could simply have the
EnsembleAttackTabDDPMModelRunnerinit take a path to the configuration file. Then you could load the file and do all of the steps to properly constructEnsembleAttackTabDDPMTrainingConfigobject within the runner class? That way a user doesn't have to do that themselves.It's possible I'm missing something where that would be a bad idea though 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if my explanation of what I was trying to suggest isn't clear. We can talk about it together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only problem I see with that is the attack changes the config a few times so any runner must have a config object in order for this to run properly (not great practice, though, I want to refactor that out at some point). I think I can use OOP to our advantage here and add the requirement to have a config for any classes implementing a model runner. TBD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Okay, I'll defer to your judgement/plan here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It ended up being a bigger change than I wanted but it looks simpler now. The only downside I see is the code inside the main library being more dependent on the config file.
I think we should move away from stuff being set in config files and move into parameters with default values or config classes. It will make it easier for people outside our team to use the library. It's a bigger effort, though, but I plan to do it little by little as I work on experiments. We can talk more in our 1 on 1 today.