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

Mistobaan/add docwebsite #274

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mistobaan
Copy link
Contributor

@Mistobaan Mistobaan commented Feb 3, 2023

First iteration on the new website documentation
image

https://trlx-docs-preview.readthedocs.io/en/latest/

@Mistobaan Mistobaan force-pushed the Mistobaan/add-docwebsite branch 3 times, most recently from 9af30de to d65cc5f Compare February 4, 2023 06:05
@Mistobaan Mistobaan marked this pull request as ready for review February 4, 2023 20:22
@maxreciprocate
Copy link
Collaborator

Can you state what are the main changes done here? Also it seems that you accidentally reverted a few prior commits off of current master

setup.py Outdated
"flake8 >= 3.8.3",
]
extras["docs"] = ["sphinx==4.0.0", "sphinx_rtd_theme"]
extras["test_prod"] = ["pytest", "pytest-xdist", "pytest-subtests", "parameterized"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this get us over just using setup.cfg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setup.cfg is actually the right way. will fix.

@Mistobaan Mistobaan force-pushed the Mistobaan/add-docwebsite branch from 88b6fda to f49a6d9 Compare February 11, 2023 01:43
Copy link
Collaborator

@jon-tow jon-tow left a comment

Choose a reason for hiding this comment

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

@Mistobaan This looks great 🚀 I left a few comments and change requests when you get a chance. Thank you!

setup.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
docs/pipeline.rst Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
Comment on lines 23 to 31
1. Setup your environment:

```bash
conda create -n trlx python=3.8 torch torch-cuda=11.7 -c pytorch -c nvidia
git clone https://github.com/CarperAI/trlx
cd trlx
pip install -e ".[dev]"
pre-commit install
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this. Most of it is repeated below.

requirements.txt Outdated Show resolved Hide resolved
dictionary.txt Show resolved Hide resolved
docs/glossary.md Show resolved Hide resolved
@jon-tow
Copy link
Collaborator

jon-tow commented Mar 2, 2023

Hi, @Mistobaan! Is there anywhere that I can lend a hand to help push this along?

@Mistobaan Mistobaan force-pushed the Mistobaan/add-docwebsite branch from 9146e8b to dd0ecc7 Compare March 5, 2023 20:44
@Mistobaan
Copy link
Contributor Author

@jon-tow I have to just find some time to reduce the patch and make it more manageable. The first delta is always tricky then the next documentations should be fine

@Mistobaan Mistobaan force-pushed the Mistobaan/add-docwebsite branch from dd0ecc7 to 77d6ba2 Compare March 20, 2023 17:00
@Mistobaan
Copy link
Contributor Author

Simplified the PR, latest preview here: https://trlx-docs-preview.readthedocs.io/en/latest/

@Mistobaan Mistobaan force-pushed the Mistobaan/add-docwebsite branch from 77d6ba2 to c5cfc13 Compare March 20, 2023 18:06
Copy link
Collaborator

@jon-tow jon-tow left a comment

Choose a reason for hiding this comment

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

Added a few more nits and cleanups to reflect recent changes. The preview docs look great!

docs/configs.rst Show resolved Hide resolved
docs/configs.rst Show resolved Hide resolved
docs/examples.md Show resolved Hide resolved
docs/README.md Show resolved Hide resolved
@Mistobaan Mistobaan force-pushed the Mistobaan/add-docwebsite branch from 07dc763 to c5cfc13 Compare March 24, 2023 05:40
@LouisCastricato
Copy link
Contributor

Hey what are we doing here

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.

5 participants