Skip to content

fix(gen): add missing defaults in ReasoningBlock #197

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.

Sign up for GitHub

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

Open
mwien wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(gen): add missing defaults in ReasoningBlock #197

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/gen/gen_ai_hub/orchestration_v2/models/message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ class ReasoningBlock(BaseModel):
content: The reasoning content.
signature: Signature string for hidden reasoning content.
"""
content: str
signature: str
content: str = ""
signature: str = ""
Comment on lines +70 to +71

@alpkom alpkom Oct 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't we define them as optional, if they're optional in the API spec?

@yamaceay yamaceay Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, introducing Optional[str] type would be different from the spec or not?

@mwien mwien Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure what the advantage would be. Now we have the following behaviour for the attributes (if I understand correctly):

  • if we get a str, then the attribute is populated with that str
  • if we don't get a value for the attribute, it is set to the default value ""

If we define content: Optional[str] = "", then again:

  • if we get a str, then the attribute is populated with that str
  • if we don't get a value for the attribute, it is set to the default value ""
  • it could only be set to None if the response explicitly contains a null value which according to the spec cannot happen

@OscarDDD OscarDDD Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, from the spec, I think this value should not be None.

@mwien mwien Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Though arguably in requests content: Optional[str] = None does make sense. ReasoningBlock is both part of responses and requests (in messages history) if I understand correctly. However, the use in requests is limited to sending it back exactly as received: https://help.sap.com/docs/sap-ai-core/generative-ai/reasoning?q=reasoning#multi-turn-conversations

@mwien mwien Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update: I read up a bit on this topic and it seems that the default value in OpenAPI specs only has meaning for request parameters and doesn't mean anything for response properties. I think both options are ok.




Expand Down
Loading