Skip to content

Conversation

@LeetJoe
Copy link
Contributor

@LeetJoe LeetJoe commented Jun 15, 2023

README is also updated.

@LeetJoe
Copy link
Contributor Author

LeetJoe commented Jun 15, 2023

@microsoft-github-policy-service agree

@conglongli conglongli self-assigned this Jun 15, 2023
@conglongli
Copy link
Contributor

Hi @LeetJoe thank you for the contribution. Please see my comments above which need your fixes. In addition, the formatting test failed and please see my comments in this PR #398 about how to fix the format.

@LeetJoe
Copy link
Contributor Author

LeetJoe commented Jun 16, 2023

Hi @conglongli thank you for your reviewing and you really offer me some good advices.
I have made some modifications following your advices and please check it again.

@conglongli
Copy link
Contributor

@LeetJoe Most of my comments are resolved, but there are two new issues. Also you missed my ask of fixing the formatting issue. If formatting is not fixed, we can't merge it
image

@LeetJoe
Copy link
Contributor Author

LeetJoe commented Jun 17, 2023

@conglongli I have fixed the problems you mentioned above. Thanks for your patience.

@conglongli
Copy link
Contributor

@LeetJoe formatting is still failling, please do exactly the following:

go to the repo
pre-commit install
pre-commit run --files all_files_you_modified
git commit and push all changes made by pre-commit

@LeetJoe
Copy link
Contributor Author

LeetJoe commented Jun 17, 2023

@conglongli I check the failure and find there is a tool called yapf and I have used it reformat my code.

@conglongli conglongli merged commit 9d12674 into deepspeedai:master Jun 17, 2023
@conglongli
Copy link
Contributor

@LeetJoe yapf is just one of the checks we did. This time it was the only format issue so that I can merge. In future I highly recommend you install and use pre-commit.

@LeetJoe
Copy link
Contributor Author

LeetJoe commented Jun 17, 2023

I have followed the code you provided above, pre-commit is installed now.
Happy to learn a lot from this review.
Thanks.

hwchen2017 pushed a commit that referenced this pull request Jun 8, 2025
* fix gitignore

* add local dataset dir

* add ignore

* add local dataset support

* add some about local dataset in README.md

* fix some

* add some in README

* remove data dir

* add line to gitignore

* fix some following good advices

* fix some about format

* reformat code using yapf

---------

Co-authored-by: 宋超 <sc@chainshome.org>
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.

2 participants