-
Notifications
You must be signed in to change notification settings - Fork 26.7k
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
Add Doc Test GPT-2 #16439
Add Doc Test GPT-2 #16439
Conversation
The documentation is not available anymore as the PR was closed or merged. |
I think this is what is required something up with CI failing code quality check?
|
Yes, you need to run Pinging @ydshieh on this PR since Patrick is on vacation this week. |
Hi, @ArEnSc Thank you for this PR! In order to run
If you haven't done this before. |
|
Hi, @ArEnSc For this sprint, you don't need to test the model, but just to test the docstrings in model files. You can see a guide here For Python files. Before you run, you need to pip install -e ".[dev]" Let me know if this works for you. |
@@ -61,7 +61,7 @@ | |||
|
|||
logger = logging.get_logger(__name__) | |||
|
|||
_CHECKPOINT_FOR_DOC = "gpt2" | |||
_CHECKPOINT_FOR_DOC = "distilgpt2" |
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 should not be changed. "gpt2" is the official checkpoint for GPT2
model, and it is used in the docstring example for GPT2Model
and GPT2LMHeadModel
.
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.
Okay got it will change back, I had thought they wanted us to use a lower resource version as per this instruction
Using a small model checkpoint instead of a large one: for example, change "facebook/bart-large" to "facebook/bart-base" (and adjust the expected outputs if any)
expected_output=[ | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
"LABEL_0", | ||
], |
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 not a helpful example at all, also, this model seems to be a sequence classification model according to its model card. The loss at 0.0 is very weird especially.
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.
Indeed, it is a text (sequence) classification model.
@ArEnSc Could you try if the following GPT2 token classification 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.
Ok I will take a look at this and give it a shot
Yes I did run the required commands specifically:
I am unsure about how to stop CI from running the add model like runner I suppose as that error came from CI |
For now, You can ignore the errors on build_pr_documentation and Add new model like template tests from the CI. |
Then used a token classification model over a sequence model 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.
Great job & well done :-)
Thank you, @ArEnSc
"Lead", | ||
"Lead", | ||
"Lead", | ||
], |
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.
Hi @sgugger
Patrick delegates the responsibility to me. I am still wondering if you have any extra comment for this PR though.
There are only 2 checkpoints on the Hub for GPT2 + token classification.
This one is trained on writing document evaluation dataset. The output for this example is therefore not really meaningful. However, I am in favor of merge as it is.
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.
@sgugger just following up on this. are we gonna move on this? and close the issue? Thanks =)
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 I missed my input was required here. Thanks for the pin @ArEnSc ! Fine by me but we should deactivate formatting for that one line to avoid wasting all that vertical space (and have the list back on one line). You can do so with a comment
# fmt: off
before and
# fmt: on
after.
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.
@sgugger will do!
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.
LGTM! Thanks for your PR!
"Lead", | ||
"Lead", | ||
"Lead", | ||
], |
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 I missed my input was required here. Thanks for the pin @ArEnSc ! Fine by me but we should deactivate formatting for that one line to avoid wasting all that vertical space (and have the list back on one line). You can do so with a comment
# fmt: off
before and
# fmt: on
after.
Hi, @ArEnSc Ping me for the merge once you finish the |
@ydshieh this one is good to go now! =) |
--> not just a hope, dream comes True now :-) Thank you again for the contribution. Merged! |
* First Pass All Tests Pass * WIP * Adding file to documentation tests * Change the base model for the example in the doc test. * Fix Code Styling by running make fixup * Called Style * Reverted to gpt2 model rather than distill gpt2 Then used a token classification model over a sequence model for an example. * Fix Styling Issue * Hopefully ignores the formatting issue. Co-authored-by: ArEnSc <[email protected]>
What does this PR do?
Fixes the broken doc tests for GPT-2
Apart of the documentation sprint work.
Fixes [Github issue] (#16292)
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
gpt2: @patrickvonplaten, @LysandreJik
Documentation: @sgugger