Skip to content
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

Resolve #218: Support soft ids as inputs for BERT/GPT2/RoBERTa/XLNet #220

Merged
merged 5 commits into from
Oct 1, 2019
Merged

Resolve #218: Support soft ids as inputs for BERT/GPT2/RoBERTa/XLNet #220

merged 5 commits into from
Oct 1, 2019

Conversation

gpengzhi
Copy link
Collaborator

Support soft_ids as inputs for encoders, classifiers, and regressors.

@gpengzhi gpengzhi changed the title Resolve #218 Support soft ids as inputs for BERT/GPT2/RoBERTa/XLNet Resolve #218: Support soft ids as inputs for BERT/GPT2/RoBERTa/XLNet Sep 30, 2019
@gpengzhi
Copy link
Collaborator Author

Resolve #218

Copy link
Collaborator

@huzecong huzecong left a comment

Choose a reason for hiding this comment

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

LGTM. What worries me is that our implementation of modules based on pre-trained stuff is a bit too repetitive, so that a seemingly small change would require modifying a bunch of files. This is also true for a lot of tests (not limited to pre-trained ones). Let's keep this in mind so we can improve this in the future.

@gpengzhi
Copy link
Collaborator Author

gpengzhi commented Oct 1, 2019

LGTM. What worries me is that our implementation of modules based on pre-trained stuff is a bit too repetitive, so that a seemingly small change would require modifying a bunch of files. This is also true for a lot of tests (not limited to pre-trained ones). Let's keep this in mind so we can improve this in the future.

This is a very good suggestion. For the code redundancy in the models, I feel that we can extract some task-specific headers for all or most of the pre-trained models. For the testing issue, some of the tests can be shared across different models. I think we can write a common test script that can be used by a group of classes for unit testing.

@huzecong
Copy link
Collaborator

huzecong commented Oct 1, 2019

That sounds good. Let's open a separate issue to keep track of this, so we can merge this PR.

@huzecong
Copy link
Collaborator

huzecong commented Oct 1, 2019

Feel free to merge when you're done with the changes.

texar/torch/losses/adv_losses.py Show resolved Hide resolved
texar/torch/modules/classifiers/gpt2_classifier.py Outdated Show resolved Hide resolved
@gpengzhi gpengzhi merged commit c6935c2 into asyml:master Oct 1, 2019
@gpengzhi gpengzhi deleted the soft-ids branch November 1, 2019 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants