Skip to content

Conversation

@aeft
Copy link
Contributor

@aeft aeft commented Sep 5, 2025

What type of PR is this?

fix: correct candle-binding replace path in go.mod files

What this PR does / why we need it:

candle-binding replace path in go.mod files is incorrect.

make test-pii-classifier will fail.

Which issue(s) this PR fixes:

Fixes #

Release Notes: No

@github-actions
Copy link

github-actions bot commented Sep 5, 2025

👥 vLLM Semantic Team Notification

The following members have been identified for the changed files in this PR and have been automatically assigned:

📁 src

Owners: @rootfs, @Xunzhuo, @wangchen615
Files changed:

  • src/training/classifier_model_fine_tuning/go.mod
  • src/training/pii_model_fine_tuning/go.mod
  • src/training/prompt_guard_fine_tuning/go.mod

This comment was automatically generated based on the OWNER files in the repository.

@netlify
Copy link

netlify bot commented Sep 5, 2025

Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit 6563b8a
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/68bb7635f0fba3000872344f
😎 Deploy Preview https://deploy-preview-65--vllm-semantic-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@rootfs
Copy link
Collaborator

rootfs commented Sep 6, 2025

@aeft thanks for spotting this! Would please also help add create an integration test suite with these these verifiers?

@netlify
Copy link

netlify bot commented Sep 6, 2025

Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit 523d3e7
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/68bc46eb9158be00086a4587
😎 Deploy Preview https://deploy-preview-65--vllm-semantic-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@rootfs rootfs merged commit 1df996f into vllm-project:main Sep 6, 2025
9 checks passed
@aeft
Copy link
Contributor Author

aeft commented Sep 6, 2025

@rootfs Sure! How about placing all tests with heavy dependencies (e.g., real model calls) into test/integration (full path: src/semantic-router/test/integration), while keeping unit tests next to the source code? This keeps the structure clear and avoids relying on go:build, similar to how large projects like Kubernetes or etcd organize their integration tests.

I also noticed that files like pkg/extproc/security_test.go are calling real models. Would it make sense to move such tests into a subdirectory under test/integration as well? Afterwards, I can update the docs and Makefile to reflect this convention.

@aeft
Copy link
Contributor Author

aeft commented Sep 6, 2025

@rootfs I noticed that these verifiers don't have assertions. How about I refactor them into tests with assertions?

@rootfs
Copy link
Collaborator

rootfs commented Sep 6, 2025

@rootfs Sure! How about placing all tests with heavy dependencies (e.g., real model calls) into test/integration (full path: src/semantic-router/test/integration), while keeping unit tests next to the source code? This keeps the structure clear and avoids relying on go:build, similar to how large projects like Kubernetes or etcd organize their integration tests.

Sounds good!

I also noticed that files like pkg/extproc/security_test.go are calling real models. Would it make sense to move such tests into a subdirectory under test/integration as well? Afterwards, I can update the docs and Makefile to reflect this convention.

Please go ahead.

Once you are done, please add new Makefile targets for these two cases and ensure make test (used by CI) can run both cases

@aeft aeft deleted the fix/go_mod_path branch September 6, 2025 17:47
@aeft
Copy link
Contributor Author

aeft commented Sep 6, 2025

@rootfs I noticed that these verifiers don't have assertions. How about I refactor them into tests with assertions?

The only concern is that it relies on a probabilistic model. However, we can exclude those examples that are ambiguous (or relax the evaluation criteria). The main purpose of this integration test is to ensure the lower bound of performance and to verify that the main logic runs correctly. If you have no objections, I'll add the corresponding assertions.

@rootfs
Copy link
Collaborator

rootfs commented Sep 6, 2025

@rootfs I noticed that these verifiers don't have assertions. How about I refactor them into tests with assertions?

The only concern is that it relies on a probabilistic model.

That's the purpose :D we want to ensure the models are reliable through CI tests

@aeft
Copy link
Contributor Author

aeft commented Sep 6, 2025

@rootfs So, would you like me to add some assertions? We can exclude those examples that are ambiguous (or relax the evaluation criteria). This ensures the lower bound of performance and verifies that the main logic runs correctly.

We can observe it first, and consider removing it if it turns out to be unstable.

@rootfs
Copy link
Collaborator

rootfs commented Sep 6, 2025

@rootfs So, would you like me to add some assertions? We can exclude those examples that are ambiguous (or relax the evaluation criteria). This ensures the lower bound of performance and verifies that the main logic runs correctly.

We can observe it first, and consider removing it if it turns out to be unstable.

I prefer assert. We can do it in two ways: for functional unit testing, relaxed or lower bound are acceptable. For integration test, we need to be more rigorous on the classification.

@aeft
Copy link
Contributor Author

aeft commented Sep 6, 2025

@rootfs So, would you like me to add some assertions? We can exclude those examples that are ambiguous (or relax the evaluation criteria). This ensures the lower bound of performance and verifies that the main logic runs correctly.
We can observe it first, and consider removing it if it turns out to be unstable.

I prefer assert. We can do it in two ways: for functional unit testing, relaxed or lower bound are acceptable. For integration test, we need to be more rigorous on the classification.

Great. For integration tests, we call the real model and test (assert) classification performance. For functional unit tests, we mock the model call and test the function logic.

I will gradually submit PRs over the next few days. Thanks for the discussion!

@rootfs
Copy link
Collaborator

rootfs commented Sep 6, 2025

Great! looking forward to your next PR

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants