Skip to content

Parse ints from floats in openai generic #1746

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 2 commits into from
Apr 8, 2025
Merged

Conversation

imalsogreg
Copy link
Contributor

@imalsogreg imalsogreg commented Apr 8, 2025

The SambaNova provider returns a float for the "completed" field of the Completions generic API. This PR makes the Completions parser more flexible, accepting both ints and floats, but always returning ints.

I have tested this locally - SambaNova and OpenAI requests now parse correctly. I didn't check in the testing code, because we don't necessarily want to mention lots of providers in our test suite.


Important

Enhance ChatCompletionGeneric to accept both int and float for created field, ensuring consistent int return using deserialize_float_to_u32.

  • Behavior:
    • ChatCompletionGeneric now accepts both int and float for created field, always returning int.
    • Adds deserialize_float_to_u32 function to handle deserialization of created field.
  • Code:
    • Adds deserialize_float_to_u32 function in types.rs to convert floats to integers using floor().
    • Updates ChatCompletionGeneric struct to use deserialize_float_to_u32 for created field.

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

Copy link

vercel bot commented Apr 8, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
baml ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 8, 2025 6:40pm

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to a9c9bcd in 2 minutes and 27 seconds

More details
  • Looked at 42 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:43
  • Draft comment:
    Consider checking for negative float values before converting to u32, as casting negative numbers may produce unexpected results.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    Since this is deserializing a Unix timestamp, negative values would be invalid and represent dates before 1970. OpenAI's API would never return a negative timestamp for a chat completion creation time. The conversion is safe in this specific context. The comment is being overly cautious about a scenario that can't occur in practice.
    I could be wrong about OpenAI's API behavior - maybe there are edge cases I'm not aware of where they might return negative values. Also, defensive programming is generally good practice.
    While defensive programming is good, we should balance it with code clarity and real-world usage. The field's semantic meaning (creation timestamp) makes negative values impossible in practice.
    The comment should be removed. The suggested check is unnecessary given that this field specifically represents a creation timestamp that will always be positive.
2. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:31
  • Draft comment:
    Add a documentation comment explaining the rationale for using f.floor() in the conversion and any assumptions about input (e.g., Unix timestamp expected to be non-negative).
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking for a documentation comment to explain the rationale behind using a specific function and any assumptions about the input. This is a request for clarification and documentation, which is not allowed according to the rules. The comment does not provide a specific code suggestion or ask for a test to be written, so it should be removed.
3. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:42
  • Draft comment:
    Using f.floor() in the conversion discards the fractional part. Confirm that flooring (rounding down) is the intended behavior for the Unix timestamp conversion.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    Unix timestamps are typically whole numbers representing seconds since epoch. If OpenAI's API is returning a float, the fractional part likely represents subsecond precision that isn't needed since the type is declared as u32. Flooring is a reasonable way to truncate to seconds. This is a common pattern when dealing with Unix timestamps. The comment is asking for confirmation without identifying any actual problem.
    The comment could be right if there's some special requirement where rounding up or rounding to nearest would be more appropriate than flooring.
    Unix timestamps are conventionally floored when truncating precision - there's no compelling reason to round differently. The code's approach follows standard practices.
    The comment should be deleted as it's asking for confirmation without identifying any actual issue, and the code follows standard practices for Unix timestamp handling.
4. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:1
  • Draft comment:
    The import use serde::de::{self, Deserializer}; includes self which isn’t used elsewhere. Consider removing it for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
5. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:14
  • Draft comment:
    Typographical error: The comment on line 14 mentions "if n is greater than 1.s". The trailing 's' after the number may be an error. Consider correcting it to simply "1" or rephrase the comment to avoid confusion.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_qrjJgbgVUu07bxd5


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 0d334bd in 1 minute and 12 seconds

More details
  • Looked at 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:35
  • Draft comment:
    Reordering the variants in the untagged enum to have Int before Float ensures that integer values are tried first during deserialization. Consider adding a comment explaining the significance of variant order in serde’s untagged enum behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The comment is technically correct - in serde, untagged enum variant order does matter for deserialization as it tries variants in order. The reordering here was intentional to try Int before Float. However, this is a small internal deserializer function, and the behavior is working as intended. Adding a comment about serde internals may be overkill for this simple case.
    The comment identifies a real technical detail that could be worth documenting. Future maintainers might not know about serde's untagged enum behavior.
    While technically accurate, this is a simple internal deserializer where the behavior is clear from the code itself. The comment suggests purely informational documentation rather than a needed code change.
    Delete the comment. While technically correct, it only suggests adding optional documentation to an internal function where the behavior is already clear from the code.
2. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:42
  • Draft comment:
    The match arms now reflect the reordered enum. This ensures that when deserializing, integers are processed first. Ensure this ordering does not unintentionally convert floats that are whole numbers if that behavior is required.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:37
  • Draft comment:
    Reordering enum variants is a good fix. Placing the Int(u32) variant first ensures that integer values are correctly matched, avoiding the potential mis-parsing of integers as floats in the untagged enum.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, explaining why a change was made. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. It simply states that the reordering is a fix and explains why.
4. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:42
  • Draft comment:
    Swapping the match arms to check for the Int variant before the Float variant further ensures correct fallback behavior. Both integer inputs and float inputs (via flooring) are now handled as intended.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the change without offering any specific guidance or questions.
5. engine/baml-runtime/src/internal/llm_client/primitive/openai/types.rs:14
  • Draft comment:
    Typographical error: The comment currently reads "if n is greater than 1.s". The trailing 's' seems to be a typo and should be removed or corrected.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_NH6eCJho3fmHo48u


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@imalsogreg imalsogreg added this pull request to the merge queue Apr 8, 2025
Merged via the queue into canary with commit 28e3d0a Apr 8, 2025
11 of 12 checks passed
@imalsogreg imalsogreg deleted the greg/samba-nova-bug branch April 8, 2025 18:40
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.

1 participant