Skip to content

Add e2e tests for embedding raw flag#16940

Draft
SamMalayek wants to merge 3 commits into
ggml-org:masterfrom
SamMalayek:feature/test-embedding-raw
Draft

Add e2e tests for embedding raw flag#16940
SamMalayek wants to merge 3 commits into
ggml-org:masterfrom
SamMalayek:feature/test-embedding-raw

Conversation

@SamMalayek

@SamMalayek SamMalayek commented Nov 2, 2025

Copy link
Copy Markdown
Contributor

🧩 Summary

This PR adds a CI workflow for end-to-end embedding CLI tests (none exist today). It establishes a small, fast, reproducible baseline for validating embedding behavior (dimensions + determinism) using tiny GGUF models.

Discussion / design context: See the companion RFC in Discussions for the longer-term plan to add a native server endpoint: #16957

⚙️ What this PR includes

  • A GitHub Actions job (embeddings.yml) that runs E2E embedding CLI tests with cached tiny models (e.g., TinyLlama).
  • Checks output dimensions and deterministic behavior.
  • Keeps runs lightweight and fast; an optional large-model stress test can be added later.

@SamMalayek
SamMalayek requested a review from CISC as a code owner November 2, 2025 16:00
@github-actions github-actions Bot added examples python python script changes devops improvements to build systems and github actions labels Nov 2, 2025
@SamMalayek
SamMalayek force-pushed the feature/test-embedding-raw branch from 015351e to 075c324 Compare November 2, 2025 18:39
@SamMalayek
SamMalayek force-pushed the feature/test-embedding-raw branch from 075c324 to c1c3d99 Compare November 2, 2025 18:55
@CISC

CISC commented Nov 2, 2025

Copy link
Copy Markdown
Member

While testing is great I have issues with this PR:

  • The CI only runs when the example code change, not when actual embedding code changes
  • The tests are not testing anything likely to break, nor indeed even anything useful

To sum up; overall as a CI job this will not run when it matters, and when it is run it will not catch any problems of consequence.

Lastly, why create a new PR instead of arguing your case in the original PR and maybe iron out concerns there?

@SamMalayek
SamMalayek requested a review from ggerganov as a code owner November 2, 2025 22:01
@SamMalayek

SamMalayek commented Nov 2, 2025

Copy link
Copy Markdown
Contributor Author

While testing is great I have issues with this PR:

  • The CI only runs when the example code change, not when actual embedding code changes
  • The tests are not testing anything likely to break, nor indeed even anything useful

To sum up; overall as a CI job this will not run when it matters, and when it is run it will not catch any problems of consequence.

I kept them in examples alongside the CLI code as a pragmatic, incremental, small first step. The intent was to expand scope and relocate to tests/e2e/embedding/ once the embedding path itself was integrated into the server runtime (or simply commonized as an intermediary step). That said, I can move them now and broaden coverage to better match expectations (done).

Lastly, why create a new PR instead of arguing your case in the original PR and maybe iron out concerns there?

That PR was closed with minimal comment. So, I started this RFC: #16957

@SamMalayek
SamMalayek force-pushed the feature/test-embedding-raw branch from e5a5b26 to 82dbeee Compare November 2, 2025 22:12
@github-actions github-actions Bot added the testing Everything test related label Nov 2, 2025
@SamMalayek
SamMalayek force-pushed the feature/test-embedding-raw branch from 82dbeee to 2de1e68 Compare November 2, 2025 23:01
@SamMalayek

Copy link
Copy Markdown
Contributor Author

Note: Empty force-pushes are to re-trigger the CI workflow following an intermittent failure.

@SamMalayek

SamMalayek commented Nov 3, 2025

Copy link
Copy Markdown
Contributor Author

Putting this in draft while work on the very needed RFC (please see) continues: #16957

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devops improvements to build systems and github actions examples python python script changes testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants