-
Notifications
You must be signed in to change notification settings - Fork 31.3k
Update installation page and add contributing to the doc #5084
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5084 +/- ##
==========================================
+ Coverage 77.24% 77.26% +0.02%
==========================================
Files 133 133
Lines 22146 22146
==========================================
+ Hits 17107 17112 +5
+ Misses 5039 5034 -5
Continue to review full report at Codecov.
|
LysandreJik
left a comment
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
docs/README.md
Outdated
| Make sure that there is a symlink from the `example` file (in /examples) inside the source folder. Run the following | ||
| command to generate it: | ||
| Make sure that there is a symlink from the `example` file (in /examples) and from the `CONTRIBUTING` guide (in the | ||
| root folder) inside the docs source folder. You shouldn't need to, but in case they are missing, run the following | ||
| commands to generate them: | ||
|
|
||
| ```bash | ||
| ln -s ../../examples/README.md examples.md | ||
| ln -s ../../CONTRIBUTING.md contributing.md |
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 wonder if these are necessary since the links are committed to the library... Just pulling the repo should automatically have the link files, shouldn't it?
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, unless a person tries on windows (symlinks would need to be recreated) but since there are make commands after that don't work on Windows, I don't think we care too much.
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.
Right, and I don't think ln is a correct command to make links under Windows?
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.
Correct, that would mklink and I think the order is reversed. I deleted this part for now.
This PR simplifies the installation page, adds the mention of TF/PT installation with one command only (for CPU) and adds a test that the installation was successful.
The tests mention is moved to CONTRIBUTING. The mention of the tokenization process for OpenAI GPT is moved to that model doc page.
It also adds the contributing guide to the documentation with a simlink (add to fix a few links to make it work).