Skip to content
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

Fix gpt-tfjs bugs, add tests and refactor code #658

Merged
merged 30 commits into from
Apr 16, 2024

Conversation

JulienVig
Copy link
Collaborator

@JulienVig JulienVig commented Apr 8, 2024

Addresses items listed in #654 #656

Fixes #656:

  • Fixes text preprocessing, now generation doesn't repeat the last input token and memory leak is fixed
  • Add test cases for gpt-tfjs
  • Create test cases for text preprocessing

Addresses parts of #654

  • Add explanation comments to gpt-tfjs code
  • Refactor classes and code organization
  • Add a compile method

Additionally

  • Add support for choosing model parameters at initialization (rather than hard coded in the task or the class definition)
  • Add support for model config serialization

@JulienVig JulienVig added bug Something isn't working discojs Related to Disco.js labels Apr 8, 2024
@JulienVig JulienVig self-assigned this Apr 8, 2024
@JulienVig
Copy link
Collaborator Author

@tharvik do you have an opinion on putting the gpt-tfjs test spec file, which relies on tfjs-node, in the discojs-core/.../gpt? Or better to put it in discojs-node?

@tharvik
Copy link
Collaborator

tharvik commented Apr 9, 2024

@tharvik do you have an opinion on putting the gpt-tfjs test spec file, which relies on tfjs-node, in the discojs-core/.../gpt? Or better to put it in discojs-node?

for now, there isn't a good place to put theses tests, it's de facto in server/tests.
with default tasks splitted from discojs-core (tracked in #647), it'll fit naturally in this "tasks" package

@JulienVig
Copy link
Collaborator Author

for now, there isn't a good place to put theses tests, it's de facto in server/tests.
with default tasks splitted from discojs-core (tracked in #647), it'll fit naturally in this "tasks" package

The thing is that this test is independent of the task, I tried to keep it as unitary as possible. It only tests gpt-tfjs implementation and capabilities

@tharvik
Copy link
Collaborator

tharvik commented Apr 9, 2024

with default tasks splitted from discojs-core (tracked in #647), it'll fit naturally in this "tasks" package

The thing is that this test is independent of the task, I tried to keep it as unitary as possible. It only tests gpt-tfjs implementation and capabilities

ho, I misread, I though it was requiring discojs-node, not tfjs-node; well done then! so yeah, having it in discojs-core makes clearly sense. you can add tfjs-node to dev deps, so that's available for building/testing but not exposed from discojs-core (sadly not enforced).

@JulienVig JulienVig marked this pull request as ready for review April 10, 2024 14:20
@JulienVig JulienVig requested a review from tharvik April 10, 2024 14:20
@JulienVig JulienVig changed the title Fix repeated token due to wrong text preprocessing Fix gpt-tfjs bugs, add tests and refactor code Apr 10, 2024
Copy link
Collaborator

@tharvik tharvik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouhou, we've a talking model, thanks!

mostly cosmetics changes, except for models/gpt changes

JulienVig and others added 3 commits April 11, 2024 14:26
Co-authored-by: Valérian Rousset <[email protected]>
Co-authored-by: Valérian Rousset <[email protected]>
@JulienVig JulienVig merged commit 3b30992 into develop Apr 16, 2024
23 checks passed
@JulienVig JulienVig deleted the 656-improve-gpt-julien branch April 16, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discojs Related to Disco.js
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gpt-tfjs only repeats the last prompt token
2 participants