Skip to content

fix: JsonSchema required #288

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

Merged
merged 1 commit into from
Apr 23, 2025
Merged

fix: JsonSchema required #288

merged 1 commit into from
Apr 23, 2025

Conversation

lvluoyue
Copy link
Contributor

No description provided.

@chr-hertel
Copy link
Member

Hi @lvluoyue, thanks for taking care of that!

How did you run into it?

I was wondering before if that's needed or only with stricr mode

@lvluoyue
Copy link
Contributor Author

Hi @lvluoyue, thanks for taking care of that!

How did you run into it?

I was wondering before if that's needed or only with stricr mode

Parameters should be optional when specifying a default value

    public function searchSong(
        string $keyword,
        #[With(minimum: 1, maximum: 60)]
        int    $limit = 10,
        #[With(minimum: 1)]
        int    $page = 1
    ): string {
        return R::mcp($this->tencentService->searchSong($keyword, $page, $limit));
    }

@chr-hertel
Copy link
Member

Sounds good, yes!

Do you mind sharing what's that R::MCP(...) - something custom or a library?

@lvluoyue
Copy link
Contributor Author

听起来不错,是的!

你介意分享那是什么 - 自定义的东西还是库?R::MCP(...)

The full name of R is response, and it is used to encapsulate various response formats into a unified format

@chr-hertel
Copy link
Member

you can run the failing tests locally with:

php vendor/bin/phpunit tests/Chain/JsonSchema/FactoryTest.php tests/Chain/Toolbox/ToolboxTest.php

@lvluoyue
Copy link
Contributor Author

lvluoyue commented Apr 21, 2025

您可以通过以下方式在本地运行失败的测试:

php vendor/bin/phpunit tests/Chain/JsonSchema/FactoryTest.php tests/Chain/Toolbox/ToolboxTest.php

ok
image

@OskarStark
Copy link
Contributor

Maybe I get it wrong but in your example the parameter is required but with a default value, so null is not supported here, or am I missing sth?

@OskarStark OskarStark changed the title fix JsonSchema required fix: JsonSchema required Apr 21, 2025
@lvluoyue
Copy link
Contributor Author

Maybe I get it wrong but in your example the parameter is required but with a default value, so null is not supported here, or am I missing sth?

I was negligent, sorry. I've modified to the correct code and modified the unit tests

@OskarStark
Copy link
Contributor

Are you using OpenAI?

@OskarStark
Copy link
Contributor

@lvluoyue
Copy link
Contributor Author

lvluoyue commented Apr 22, 2025

https://platform.openai.com/docs/guides/function-calling#strict-mode

Thanks, I'm using OpenAI, and I'm writing an mcp server with
php-llm/mcp-sdk, but it seems to be still in development, and I want it to support version 2025-03-26

https://modelcontextprotocol.io/docs/concepts/tools

@chr-hertel
Copy link
Member

if you revert commit b6007f4 the pipeline should be fine - i personally don't see real value in those @throws tags on method tbh, besides interface docs

@chr-hertel chr-hertel added the bug Something isn't working label Apr 22, 2025
@lvluoyue lvluoyue force-pushed the main branch 2 times, most recently from 174af0b to b8a7687 Compare April 23, 2025 05:35
@lvluoyue lvluoyue closed this Apr 23, 2025
@lvluoyue lvluoyue reopened this Apr 23, 2025
Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

Thanks @lvluoyue! Looks good to me now 👍

@chr-hertel chr-hertel merged commit 30b7f92 into php-llm:main Apr 23, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants