Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Added binary gender bias-mitigated RoBERTa model for SNLI #268

Merged
merged 10 commits into from
Jun 2, 2021
Merged

Added binary gender bias-mitigated RoBERTa model for SNLI #268

merged 10 commits into from
Jun 2, 2021

Conversation

ArjunSubramonian
Copy link
Contributor

@ArjunSubramonian ArjunSubramonian commented May 23, 2021 •
edited
Loading

Added binary gender bias-mitigated RoBERTa model for SNLI.

Depends on allenai/allennlp#5176.

@ArjunSubramonian ArjunSubramonian self-assigned this May 23, 2021
AkshitaB
AkshitaB reviewed May 25, 2021
View reviewed changes
Copy link
Contributor

@AkshitaB AkshitaB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, overall. Just the same question as in the other PR about needing the full allennlp.fairness.bias_mitigator_applicator.BiasMitigatorApplicator in the config.

@ArjunSubramonian
Copy link
Contributor Author

Looks good, overall. Just the same question as in the other PR about needing the full allennlp.fairness.bias_mitigator_applicator.BiasMitigatorApplicator in the config.

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.

@ArjunSubramonian ArjunSubramonian mentioned this pull request May 26, 2021
Merged
@ArjunSubramonian
Copy link
Contributor Author

Looks good, overall. Just the same question as in the other PR about needing the full allennlp.fairness.bias_mitigator_applicator.BiasMitigatorApplicator in the config.

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.

I'm just a little confused about why "snli" is not accepted as a registered DatasetReader. This is the reason why I have the full path of the reader specified. It seems fine for every other model, but I'm struggling to figure out what's causing an issue here.

AkshitaB
AkshitaB reviewed Jun 2, 2021
View reviewed changes
Copy link
Contributor

@AkshitaB AkshitaB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those modules need to be imported in your test file. In the usual case, users would include the right package using --include_package.

tests/pair_classification/models/bias_mitigator_applicator_test.py Show resolved Hide resolved
test_fixtures/pair_classification/bias_mitigation/experiment.json Outdated Show resolved Hide resolved
test_fixtures/pair_classification/bias_mitigation/experiment.json Outdated Show resolved Hide resolved
@ArjunSubramonian ArjunSubramonian enabled auto-merge (squash) June 2, 2021 23:36
@ArjunSubramonian ArjunSubramonian merged commit 5dcf2b9 into main Jun 2, 2021
@ArjunSubramonian ArjunSubramonian deleted the arjuns/bias-mitigated-snli branch June 2, 2021 23:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AkshitaB AkshitaB AkshitaB approved these changes

@epwalsh epwalsh Awaiting requested review from epwalsh

Assignees

@ArjunSubramonian ArjunSubramonian

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ArjunSubramonian @AkshitaB