-
-
Notifications
You must be signed in to change notification settings - Fork 27
[Platform] Add Groq support #115
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
base: main
Are you sure you want to change the base?
Conversation
Contributes to symfony#16
|
||
$platform = PlatformFactory::create($_SERVER['GROQ_API_KEY']); | ||
$model = new Llama(Llama::LLAMA3_70B, [ | ||
'temperature' => 0.5, // default options for the 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.
as this is the default, can we remove it in the example?
* `Meta's Llama`_ with `Azure`_, `Ollama`_, `Replicate`_, `AWS Bedrock`_ and `Groq`_ as Platform | ||
* `Google's Gemma`_ with `Groq`_ as Platform | ||
* `Mistral's Mixtral`_ with `Groq`_ as Platform | ||
* `Alibaba's Qwen`_ with `Groq`_ as Platform |
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.
Maybe we should use some kind of table for this?
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.
I agree, this doesn't scale well - not necessarily scope of this PR, but I'm in need for better ideas - even feels like a three dimensional table 😆
* @author Christopher Hertel <[email protected]> | ||
* @author Dave Hulbert <[email protected]> | ||
*/ | ||
class Llama extends 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.
could be something more neutral like
class Llama extends Model | |
class Model extends BaseModel |
since this is not only Llama it's a bit confusing - it's rather a collection of models available on Groq
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.
Thanks. Yeah, I'll update. Started with Llama, then I realised they have others now.
require_once dirname(__DIR__, 2).'/vendor/autoload.php'; | ||
(new Dotenv())->loadEnv(dirname(__DIR__).'/.env'); | ||
|
||
$platform = PlatformFactory::create($_SERVER['GROQ_API_KEY']); |
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.
when introducing a new env var, please add it to .env
file (without value in this case) and check if the variable is set before - see other examples
use Symfony\AI\Platform\Message\MessageBag; | ||
use Symfony\Component\Dotenv\Dotenv; | ||
|
||
require_once dirname(__DIR__, 2).'/vendor/autoload.php'; |
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.
The 2 can be removed here:
require_once dirname(__DIR__, 2).'/vendor/autoload.php'; | |
require_once dirname(__DIR__).'/vendor/autoload.php'; |
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.
after this change the example works for me 👍
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.
Ah, thanks. Somehow I got in a bit of a mess with doing a composer install in the monorepo!
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.
Please split the test class per tested class
/** | ||
* @covers \Symfony\AI\Platform\Bridge\Groq\Llama | ||
*/ |
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.
please use #[CoversClass(Llama::class)]
attribute instead
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.
Welcome to this repository @dave1010 - and thanks a ton for working on this! New bridges are always welcome!
@OskarStark and me added some comments, but I think mostly minor stuff - so great work already :) 👍
Please always make sure to run provided example classes yourself locally - to make sure they are functional. You can find some pointers here: https://github.com/symfony/ai/blob/main/examples/README.md
We just created some Thanks |
Will do. Thanks. Sorry, haven't had a chance to get back to this yet. |
Contributes to #16