-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 document for GenAI #7170
add document for GenAI #7170
Conversation
@NiklasGustafsson @stephentoub @SteveSandersonMS can you please take a look and provide feedback. This is the design document for the GenAI library which consolidates the models ported to TorchSharp (Phi, Llama, StableDiffusion) into a single library for easier consumption. |
{ | ||
public virtual ( | ||
Tensor, // output token ids [batch_size, sequence_length] | ||
Tensor // output logits [batch_size, sequence_length, vocab_size] |
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.
Is this the TorchSharp tensor type? Can / should it become the System.Numerics.Tensors one?
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.
Is this the TorchSharp tensor type
Yes
should it become the System.Numerics.Tensors one?
If System.Numerics.Tensors can work with libtorch, then maybe yes.
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.
We're some ways from having Tensor integrated with TorchSharp, it is still experimental and subject to change.
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.
And it would be the TorchSharp shim implementing ITensor, not Tensor from SNT.
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.
Presumably it would be ITensor<T>
such that a Tensor<T>
could be passed in? The implementation could type test if it were a TorchSharp implementation and act accordingly, presumably.
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, in the fullness of time.
where TTokenizer : ITokenizer | ||
where TCausalLM : nn.Module<CausalLanguageModelInput, CausalLanguageModelOutput> | ||
{ | ||
public CausalLMPipeline<LLama2Tokenizer, Phi3ForCasualLM> Create(LLama2Tokenizer tokenizer, Phi3ForCasualLM model); |
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.
Presumably this is intended to instead be a static method on the base class?
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.
Hmmm on the second thought, maybe not. Because CasualLMPipeline
and CasualLMPipeline<TTokenizer, TCasualLM>
will live in GenAI.Core
while Phi3ForCasualLM
lives in GenAI.Phi3
.
In that case, this Create
method is no longer necessary exist, using a constructor to take TTokenzier
and TCausalLM
would be easier to understand
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
We are excited to review your PR.
So we can do the best job, please check:
Fixes #nnnn
in your description to cause GitHub to automatically close the issue(s) when your PR is merged.Add desgin document for Add GenAI packages #7169