Improvements by adrienbrault · Pull Request #1 · CodeRevolutionPlugins/GPT-3-Encoder-PHP

adrienbrault · 2023-02-03T15:03:19Z

You should also add the package to https://packagist.org

cosmastech

This is awesome! Thanks for doing it.

I have added some suggestions to make it a bit more extensible and add in some PHP features from 8.0+.

I'm of course not the package maintainer, just some things I saw that might be helpful.

cosmastech · 2023-03-15T11:58:45Z

src/Encoder.php

+
+    public static function create(): self
+    {
+        $raw_chars = file_get_contents(dirname(__FILE__) . "/../data/characters.json");


Would it make sense to have the filepaths passed as parameters?

cosmastech · 2023-03-15T12:01:51Z

src/Encoder.php

+    public static function create(): self
+    {
+        $raw_chars = file_get_contents(dirname(__FILE__) . "/../data/characters.json");
+        $byte_encoder = json_decode($raw_chars, true);


Might be convenient to have JSON_THROW_ON_ERROR as the fourth parameter

cosmastech · 2023-03-15T12:04:25Z

composer.json

    "minimum-stability": "stable",
-    "require": {}
+    "require": {
+        "php": ">7.4"


Any particular reason to go with such an old PHP version? 7.4 is past its EOL. I would suggest just going with 8.0 here (security patches stopping in November of this year), if not 8.1 or 8.2

cosmastech · 2023-03-15T12:08:06Z

composer.json

+    },
    "minimum-stability": "stable",
-    "require": {}
+    "require": {


should ext-mbstring also be added?

cosmastech · 2023-03-15T12:09:09Z

gpt3-encoder.php

-    }
-    return -1;
-}
+use CodeRevolutionPlugins\GPT3Encoder\Encoder;


This whole file would serve better as an example in a README

cosmastech · 2023-03-15T12:26:35Z

src/Encoder.php

+
+    public function encode(string $text): array
+    {
+        $bpe_tokens = array();


Suggested change

$bpe_tokens = array();

$bpe_tokens = [];

cosmastech · 2023-03-15T12:27:55Z

src/Encoder.php

+        preg_match_all("#'s|'t|'re|'ve|'m|'ll|'d| ?\p{L}+| ?\p{N}+| ?[^\s\p{L}\p{N}]+|\s+(?!\S)|\s+#u", $text, $matches);
+        if(!isset($matches[0]) || count($matches[0]) == 0)
+        {
+            error_log('Failed to match string: ' . $text);


I think it makes sense to throw a custom RuntimeException here and let the caller determine how to handle it.

cosmastech · 2023-03-15T12:28:07Z

src/Encoder.php

+            return $bpe_tokens;
+        }
+
+        $cache = array();


Suggested change

$cache = array();

$cache = [];

cosmastech · 2023-03-15T12:28:25Z

src/Encoder.php

+            $new_tokens = array();
+            $chars = array();


Suggested change

$new_tokens = array();

$chars = array();

$new_tokens = $chars = [];

cosmastech · 2023-03-15T12:29:13Z

src/Encoder.php

+
+    private function split($str, $len = 1)
+    {
+        $arr		= [];


should have consistent formatting.

adrienbrault · 2023-03-15T12:32:54Z

This is awesome! Thanks for doing it.

I have added some suggestions to make it a bit more extensible and add in some PHP features from 8.0+.

I'm of course not the package maintainer, just some things I saw that might be helpful.

I am not planning on working on this MR anymore.

I am using https://github.com/Gioni06/GPT3Tokenizer now

cosmastech · 2023-04-20T12:43:14Z

This is awesome! Thanks for doing it.
I have added some suggestions to make it a bit more extensible and add in some PHP features from 8.0+.
I'm of course not the package maintainer, just some things I saw that might be helpful.

I am not planning on working on this MR anymore.

I am using https://github.com/Gioni06/GPT3Tokenizer now

Thanks for the tip! 👍

The PHP alternative is discontinued, its author is using the project to which this patch points to, which is also available in the PHP package manager. See: CodeRevolutionPlugins/GPT-3-Encoder-PHP#1 (comment)

adrienbrault force-pushed the improvements branch from b443113 to 0d6e4b3 Compare February 3, 2023 15:13

refactor: autoloading and a class api

1838c9e

adrienbrault force-pushed the improvements branch from 0d6e4b3 to 1838c9e Compare February 3, 2023 15:22

adrienbrault marked this pull request as ready for review February 3, 2023 15:25

cosmastech reviewed Mar 15, 2023

View reviewed changes

	$new_tokens = array();
	$chars = array();
	$new_tokens = $chars = [];

Conversation

adrienbrault commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cosmastech left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrienbrault commented Mar 15, 2023

Uh oh!

cosmastech commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adrienbrault commented Feb 3, 2023 •

edited

Loading

cosmastech left a comment •

edited

Loading