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

add bunch of cleanups and design principle section #71

Merged
merged 4 commits into from
Feb 23, 2024
Merged

Conversation

wanchaol
Copy link
Contributor

No description provided.

clena up the repo to:

1. remove unused files
2. delete not used code comments
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Feb 23, 2024
README.md Outdated

## Design Principles

TorchTrain is a native PyTorch library with various training techniques, it utilizes the PyTorch ecosystem for things like data loading (i.e. HuggingFace datasets), the core functionality is written in PyTorch.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit on grammar:
TorchTrain is a native PyTorch library with various training techniques. While it utilizes the PyTorch ecosystem for things like data loading (i.e. HuggingFace datasets), the core functionality is written in PyTorch.

README.md Outdated
@@ -67,3 +67,12 @@ If your gpu count per node is not 8, adjust:
```#SBATCH --gpus-per-task```

in the SBATCH command section.


## Design Principles
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this go near the top of the readme? It's a (good) intro and design/architecture guide, I would not put it at the bottom.

Copy link
Contributor

@lessw2020 lessw2020 left a comment

Choose a reason for hiding this comment

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

lgtm - I like the guiding 'vision' it sets out.
two nits - one grammar, the other I think the readme addition should be near the top for users to see it up front.

@wanchaol wanchaol merged commit b4cda94 into main Feb 23, 2024
3 checks passed
philippguevorguian pushed a commit to YerevaNN/YNNtitan that referenced this pull request Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants