Skip to content

Add new model docs#9667

Merged
patrickvonplaten merged 26 commits into
huggingface:masterfrom
patrickvonplaten:add_new_model_temp
Feb 1, 2021
Merged

Add new model docs#9667
patrickvonplaten merged 26 commits into
huggingface:masterfrom
patrickvonplaten:add_new_model_temp

Conversation

@patrickvonplaten
Copy link
Copy Markdown
Contributor

@patrickvonplaten patrickvonplaten commented Jan 18, 2021

What does this PR do?

This PR adds more information on how to add a model to Transformers docs.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

UPDATE

The model_doc/add_new_model.rst is now finished for a first merge IMO. It would be amazing if @LysandreJik @sgugger you could review the file real quick again - I tried to add all of your suggestions. Also, I added a diagram showing the model design of Transformers - which was not reviewed yet. Note that I did not add a clear design for Tokenizers since it takes a lot of time to do so and I want to iteratively improve this step-by-step explanation. The first model, for which I'd like to mentor someone from the community would also be BigBird which does not need a new tokenizer.

In addition, I would be extremely grateful if @stas00 @abhishekkrthakur @patil-suraj @stefan-it @NielsRogge you have 10 minutes review the model_doc/add_model.rst file for possible improvements since you guys just recently added a new model. Your feedback would be especially useful since you might have a much more "unbiased" view what is difficult/easy when adding a model.

Copy link
Copy Markdown
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Fantastic work! Thank you for putting all of that into words.

I really love how enthusiastic it feels, with the 🎉 You are Awesome! 😎 and such!

Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Copy link
Copy Markdown
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

This is a great template, thanks for adding!

Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.md Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Copy link
Copy Markdown
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

Boy, this was a monumental work, @patrickvonplaten! Absolutely phenomenal work!

I left a bunch of small suggestions - please feel free to ignore any or all, no need for any justifications.

Wrt delivery, I felt it may be a bit overwhelming with the amount of information. So I highly recommend to add numbers to the sub-sections so that it's easier for one to know where they are at. It's super-detailed, which is great, but also one could get scared with so many details. Perhaps even adding a clear Table of Contents with numbered items, so the reader knows, ok, I'm at 6/10, like in a book.

In my experience I used a very different approach to porting, because I have a hard time working in vacuum which this approach suggests. To me I need to see a constant success, while progressing towards a bigger goal. So my approach was to have 2 code bases side-by-side, with 2 small scripts, using real text and not just a few numbers and porting the tokenizer first, so I at each step I was matching the ported code with the original, rather than work on each separately.
That's why it was important for me to choose checkpoints that used languages I speak.

But there is more than one way to skin-the-cat, and my approach is documented in my blog post, albeit it's outdated now since many things have changed in the code layout since it was written.

Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst
@patrickvonplaten patrickvonplaten changed the title [WIP] Add new model docs Add new model docs Feb 1, 2021
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
patrickvonplaten and others added 4 commits February 1, 2021 13:01
Co-authored-by: Stas Bekman <[email protected]>
Co-authored-by: Bram Vanroy <[email protected]>
Co-authored-by: Bram Vanroy <[email protected]>
Co-authored-by: Stas Bekman <[email protected]>
Co-authored-by: Stas Bekman <[email protected]>
Co-authored-by: Stefan Schweter <[email protected]>
Co-authored-by: Bram Vanroy <[email protected]>
Copy link
Copy Markdown
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Left some last typo-fixes suggestions. In general remember that in rst you can't have nested formatting (like code-formatting inside a bold block for instance).

Also this is super mega nitty but you use indiscriminately Object and :obj:Object. In a docstring they end up both in bold and code-formatting but in a rst file the first is jsut code-formatting and the second is bold + code formatting.

Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
Comment thread docs/source/add_new_model.rst Outdated
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.

7 participants