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(model): update start_date type to datetime and make unit optional #1068

Closed

Conversation

Julian-Schaefer
Copy link

@Julian-Schaefer Julian-Schaefer commented Jan 12, 2025

Fixes langfuse/langfuse#4973


Important

Update start_date to datetime and make unit optional in Model class.

  • Model Class Updates:
    • Change start_date type from date to datetime in Model class.
    • Make unit field optional in Model class.

This description was created by Ellipsis for 6c6cee3. It will automatically update as commits are pushed.

@CLAassistant
Copy link

CLAassistant commented Jan 12, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

Updates the Model class schema to support datetime precision for start dates and optional usage units, improving flexibility in model definitions.

  • Changed start_date type from Optional[dt.date] to Optional[dt.datetime] in /langfuse/api/resources/commons/types/model.py for more granular timestamp support
  • Modified unit field from ModelUsageUnit to Optional[ModelUsageUnit] with default None to allow model definitions without usage units
  • Maintains backward compatibility through existing datetime serialization in Config class

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +34 to 37
unit: typing.Optional[ModelUsageUnit] = pydantic_v1.Field(default=None)
"""
Unit used by this model.
"""
Copy link

Choose a reason for hiding this comment

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

logic: Making unit optional may affect cost calculations if code assumes unit is always present. Ensure all consumers handle None case.

Copy link
Member

@marcklingen marcklingen left a comment

Choose a reason for hiding this comment

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

Thanks for opening this pr

@hassiebp this needs fix in fern definition as this is autogenerated

@marcklingen marcklingen requested a review from hassiebp January 13, 2025 08:56
@hassiebp
Copy link
Contributor

Thanks for your contribution @Julian-Schaefer - I have used your suggested changes in #1071 and marked you as a co author. Welcome on the contributors list! https://github.com/langfuse/langfuse-python/releases/tag/v2.57.6

@hassiebp hassiebp closed this Jan 13, 2025
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.

bug: pydantic ValidationError in models.list()
4 participants