search

bigscience-workshop / Megatron-DeepSpeed

Ongoing research training transformer language models at scale, including: BERT & GPT-2
Other
1.31k stars 213 forks source link

Followup PR for adding generation-server #339

Closed mayank31398 closed 2 years ago

mayank31398 commented 2 years ago

@stas00 This will fix:

  1. documentation (currently outdated)
  2. fix MII for using DS-inference in server deployments

Let me know if more functionality is desired. Followup for https://github.com/bigscience-workshop/Megatron-DeepSpeed/pull/328

stas00 commented 2 years ago

super, do you want to keep this one open and continue pushing fixes into it? or merge if this feels complete in a bit and then iterate if more is needed?

mayank31398 commented 2 years ago

I would like to keep this open. At least till the above 3 things are fixed. Maybe we can add newer things in subsequent PRs.

stas00 commented 2 years ago

Sounds good to me, @mayank31398

I'm also tweaking all 3 scripts to support all the different options: https://github.com/bigscience-workshop/Megatron-DeepSpeed/pull/340 while running many benchmarks, including int8 support.

mayank31398 commented 2 years ago

I already have int8 benchmarks running using the benchmark scripts. @stas00 I am not sure if we need the scripts anymore? Is the goal to move the server PRs to another project and retain the scripts here? Also, had a talk with @jeffra related to this. Maybe we can decide in the future.

stas00 commented 2 years ago

I already have int8 benchmarks running using the benchmark scripts. @stas00 I am not sure if we need the scripts anymore?

We are working on a blog https://github.com/huggingface/blog/pull/432 and I'd like to have really simple one-script contains all demos for it. That's what the scripts for. Easy quick read w/o multiple imports.

Your abstraction re-work is fantastic for production and extension, but doesn't lend for quick understanding.

So in my mind both sets are needed as they serve different needs.

Is the goal to move the server PRs to another project and retain the scripts here? Also, had a talk with @jeffra related to this. Maybe we can decide in the future.

Honestly, I'm not quite sure. We can move them to the research projects folder under transformers, we can keep them here, you can even make a whole separate repo out of your work and distribute it via pip / conda if it resonates with you.

wrt blog - I'm not yet sure if it's best to have 2 separate posts - one for scripts and another for server, as Nicolas has developed a whole other solution using Rust (see the blog). Perhaps your and his solutions would go well together as a post dedicated to the server solutions - i.e. you would co-author it.

I'm just brainstorming here, so please make suggestions / throw ideas and let's land these somewhere where they would be most useful to the user.

But I also need to move on with other projects so I would like to finish this in the next few days if possible. (I mean the scripts, please take your time with the server code as much as you need - no rush)

mayank31398 commented 2 years ago

Yeah, I think I understand :)

mayank31398 commented 2 years ago

Ready for merge @stas00

stas00 commented 2 years ago

@mayank31398, FYI I'm in the process of moving these 2 code sets to a dedicated repo: https://github.com/huggingface/transformers-bloom-inference/ as these have nothing to do with this current repo.

All moved.

The problem is that it appears that your github user setup is somewhat broken, as you can see e.g. here and here:

github only shows your name and no link to your username. I think it's because of your email setup when you commit - github doesn't know your email address and can't associate it with your github username.

I tried to import your code while giving you full credits with:

git commit --author "Mayank Mishra <mayankmishra@Mayanks-MacBook-Pro.local>" -m "move the server solution by Mayank" bloom-inference-server

and ended up with the same issue:

https://github.com/huggingface/transformers-bloom-inference/commit/b8a64d99ffd78b840a7481ec5bf5d40a6ab40e6a

your name is there, but no link to your username in the commit description.

I took the info from your commits.

If you don't care it's fine with me. But if you fix this then we can fix the commit so that it's clear you authored that code.

Otherwise currently anybody reading your contribution is unlikely to find you...

I will shortly add you to the repo so that you could push there directly. If you have further questions please don't hesitate to ask.

I also edited your section's README to add a support sub-section - please feel free to further edit it to your liking.

mayank31398 commented 1 year ago

I have fixed the username problem @stas00 ^^ Is it possible to ammend commit in the new repo now? Now, it shows my username: https://github.com/mayank31398/Papers-books-and-blogs/commits/master

mayank31398 commented 1 year ago

I didn't even realize my git was setup incorrectly. Thanks for letting me know :)

mayank31398 commented 1 year ago

Thanks for modifying

stas00 commented 1 year ago

I'm not sure I did it in the cleanest way, but it seemed to work. You'd need to git pull to rebase your clone as it did end up changing history.

I will try a different approach in the future as this is definitely not the best way for a repo with many devs/users.

https://stackoverflow.com/questions/3042437/how-to-change-the-commit-author-for-a-single-commit

towards the end there are simpler solutions that amend the author globally