-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Conversation
…s/post-processing-debiasing
…s/contextualized-bias-mitigation
…s/contextualized-bias-mitigation
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.
Just a couple of comments so far.
# Currently doing this manually because difficult to | ||
# dynamically forward __getattribute__ due to | ||
# behind-the-scenes usage of dunder attributes by torch.nn.Module | ||
# and both BiasMitigatorWrapper and base_model inheriting from Model |
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.
I've seen this done before, so should be possible. See https://github.com/facebookresearch/fairscale/blob/b54eed1bd039e9ac73b82947e85113f78805c8eb/fairscale/nn/data_parallel/fully_sharded_data_parallel.py#L567-L572 for an example.
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.
We had a discussion about this. This was my initial implementation, but the problem is that I want all function calls (not just the missing ones) to be delegated to base_model
, but all non-function attributes to be the ones that belong to bias_mitigator_applicator
. There is __getattribute__
(__getattr__
is only for missing attribute calls), and I tried messing around with this, but because torch.nn.Module
does some behind-the-scenes things with __getattribute__
, this created so many stack overflow errors (__getattribute__
kept calling itself), and I didn't feel comfortable including that in the library. I think this is why you had recommended just manually delegating calls. I explored this for way longer than I should have lol (probably 10 hours), and there's just not a good way to identify if something is a function in Python; you can only tell if something seems like a callable.
@Subcommand.register("evaluate-bias-mitigation") | ||
class EvaluateBiasMitigation(Subcommand): |
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.
We usually define subcommands in allennlp.commands
. You could just move this file over there.
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.
I'm considering just not including this in the core library because it's too specific of a command, @AkshitaB had the great idea to keep it in the guide, so I will just remove it from this PR.
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.
@ArjunSubramonian I think you missed this. :)
@@ -0,0 +1,122 @@ | |||
from typing import Dict, Optional |
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.
Was this just copied over from allennlp-models
?
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.
yes, it's used in the test, but the environment in which the test is run doesn't have allennlp-models
.
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's fine to have some tests in allennlp-models
for this code. For instance, we have some checklist tests there, because that's where the models are.
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.
I added the test under allennlp-models
.
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.
We can remove this file now, right?
allennlp/commands/__init__.py
Outdated
@@ -20,6 +20,7 @@ | |||
from allennlp.common.plugins import import_plugins | |||
from allennlp.common.util import import_module_and_submodules | |||
from allennlp.commands.checklist import CheckList | |||
from allennlp.fairness.evaluate_bias_mitigation import EvaluateBiasMitigation |
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.
Are we including this?
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.
Sorry, we are not!
@@ -19,3 +19,4 @@ | |||
from allennlp.data.dataset_readers.sequence_tagging import SequenceTaggingDatasetReader | |||
from allennlp.data.dataset_readers.sharded_dataset_reader import ShardedDatasetReader | |||
from allennlp.data.dataset_readers.text_classification_json import TextClassificationJsonReader | |||
from allennlp.data.dataset_readers.snli import SnliReader |
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 is present in allennlp-models
right? Do we need to have it 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.
yes, it's used in the test, but the environment in which the test is run doesn't have allennlp-models
.
""" | ||
|
||
def __init__(self): | ||
self.direction = None |
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.
We should specify the expected types for direction
and noise
.
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.
Done!
raise NotImplementedError | ||
|
||
|
||
# TODO: remove equalize words from evaluation words |
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.
Is this for later, or for this PR?
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 is for later, the current assumption is that the user won't have any overlap between the embeddings for which to mitigate bias and the equalize set.
self, | ||
seed_words_file: Union[PathLike, str], | ||
tokenizer: Tokenizer, | ||
direction_vocab: Vocabulary = None, |
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.
direction_vocab: Optional[Vocabulary] = None
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.
Have done this for all instances of Vocabulary = None
"validation_data_path": "test_fixtures/fairness/snli_dev.jsonl", | ||
"test_data_path": "test_fixtures/fairness/snli_test.jsonl", | ||
"model": { | ||
"type": "allennlp.fairness.bias_mitigator_applicator.BiasMitigatorApplicator", |
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.
Does it need to be a full path?
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.
In this case, yes, because BiasMitigatorApplicator is in allennlp.fairness and FromParams only looks at the modules imported in model's init.py. I tried placing BiasMitigatorApplicator in model's init.py, but this caused a circular dependency issue for some reason - BiasMitigatorApplicator was using some fairness module classes that hadn't been imported yet, etc.
Co-authored-by: Pete <[email protected]>
…b.com/allenai/allennlp into arjuns/contextualized-bias-mitigation
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.
Minor comments.
@@ -0,0 +1,122 @@ | |||
from typing import Dict, Optional |
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.
We can remove this file now, right?
@Subcommand.register("evaluate-bias-mitigation") | ||
class EvaluateBiasMitigation(Subcommand): |
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.
@ArjunSubramonian I think you missed this. :)
* added linear and hard debiasers * worked on documentation * committing changes before branch switch * committing changes before switching branch * finished bias direction, linear and hard debiasers, need to write tests * finished bias direction test * Commiting changes before switching branch * finished hard and linear debiasers * finished OSCaR * bias mitigators tests and bias metrics remaining * added bias mitigator tests * added bias mitigator tests * finished tests for bias mitigation methods * fixed gpu issues * fixed gpu issues * fixed gpu issues * resolve issue with count_nonzero not being differentiable * added more references * fairness during finetuning * finished bias mitigator wrapper * added reference * updated CHANGELOG and fixed minor docs issues * move id tensors to embedding device * fixed to use predetermined bias direction * fixed minor doc errors * snli reader registration issue * fixed _pretrained from params issue * fixed device issues * evaluate bias mitigation initial commit * finished evaluate bias mitigation * handles multiline prediction files * fixed minor bugs * fixed minor bugs * improved prediction diff JSON format * forgot to resolve a conflict * Refactored evaluate bias mitigation to use NLI metric * Added SNLIPredictionsDiff class * ensured dataloader is same for bias mitigated and baseline models * finished evaluate bias mitigation * Update CHANGELOG.md * Replaced local data files with github raw content links * Update allennlp/fairness/bias_mitigator_applicator.py Co-authored-by: Pete <[email protected]> * deleted evaluate_bias_mitigation from git tracking * removed evaluate-bias-mitigation instances from rest of repo * addressed Akshita's comments * moved bias mitigator applicator test to allennlp-models * removed unnecessary files Co-authored-by: Arjun Subramonian <[email protected]> Co-authored-by: Arjun Subramonian <[email protected]> Co-authored-by: Arjun Subramonian <[email protected]> Co-authored-by: Arjun Subramonian <[email protected]> Co-authored-by: Akshita Bhagia <[email protected]> Co-authored-by: Pete <[email protected]>
Changes proposed in this pull request: