Skip to content

v0.5.2 add error chunk, remove pipeline and sem-search SDK resource, add agent configurations #103

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 14 commits into from
Apr 22, 2025

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Apr 20, 2025

Important

Enhance Laminar SDK with improved error handling, updated agent configurations, removal of deprecated resources, and new Google GenAI instrumentation.

  • Behavior:
    • Add error handling for agent responses in async_client.py and sync_client.py, including handling of error and timeout chunk types.
    • Remove Pipeline and SemanticSearch resources from both asynchronous and synchronous clients.
    • Update agent configuration options in agent.py to include agent_state, storage_state, timeout, cdp_url, max_steps, thinking_token_budget, and start_url.
  • Instrumentation:
    • Add Google GenAI instrumentation in google_genai/__init__.py, config.py, and utils.py.
    • Update Instruments enum in instruments.py to include GOOGLE_GENAI.
  • Dependencies:
    • Update pyproject.toml to restrict pydantic to <3.0.0 and httpx to >=0.25.0.
    • Update OpenTelemetry instrumentation dependencies to >=0.39.2.
  • Misc:
    • Rename chunkType to chunk_type in README.md and types.py.
    • Add ignore_inputs parameter to observe() decorator in decorators.py and update related tests in test_observe.py.
    • Bump version to 0.5.2 in version.py.

This description was created by Ellipsis for 53ff61d. You can customize this summary. It will automatically update as commits are pushed.

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.

Caution

Changes requested ❌

Reviewed everything up to 2759846 in 2 minutes and 12 seconds. Click for details.
  • Reviewed 1453 lines of code in 18 files
  • Skipped 0 files when reviewing.
  • Skipped posting 25 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. README.md:226
  • Draft comment:
    Updated from 'chunkType' to 'chunk_type' in examples. Ensure consistency with SDK API.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. pyproject.toml:55
  • Draft comment:
    Dependency versions updated to 0.39.2. Verify compatibility for all extras.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is about dependency versions and asks the author to verify compatibility, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue.
3. src/lmnr/__init__.py:9
  • Draft comment:
    Removed exports for ChatMessage, NodeInput, PipelineRunError, and PipelineRunResponse. Confirm no consumers rely on them.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to confirm that no consumers rely on the removed exports. This is a request for confirmation, which is not allowed according to the rules. The comment does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
4. src/lmnr/openllmetry_sdk/__init__.py:73
  • Draft comment:
    New static shutdown() added to TracerManager. Great for cleanup—ensure it's called at proper lifecycle points.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that the new static shutdown() method is called at the correct lifecycle points. This is a general request for verification and does not provide a specific suggestion or point out a specific issue. It violates the rule against asking the author to ensure behavior is intended or tested.
5. src/lmnr/openllmetry_sdk/tracing/tracing.py:237
  • Draft comment:
    TracerWrapper.shutdown() now only calls tracer_provider.shutdown(), removing force_flush/shutdown on spans_processor. Verify that spans are correctly flushed before shutdown if needed.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50% The comment is asking the author to verify that spans are correctly flushed before shutdown, which is against the rules as it asks for verification. However, it does point out a specific change in behavior that could be important, so it might be useful to rephrase it to suggest checking the behavior without directly asking for verification.
6. src/lmnr/sdk/client/asynchronous/async_client.py:66
  • Draft comment:
    Pipeline and SemanticSearch resources have been removed; ensure that clients are updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that clients are updated due to the removal of resources. This falls under asking the author to ensure something is done, which is against the rules.
7. src/lmnr/sdk/client/asynchronous/resources/agent.py:38
  • Draft comment:
    New parameters (agent_state, storage_state, return_agent_state, return_storage_state, timeout, cdp_url, max_steps, thinking_token_budget) added to run(). Update documentation and ensure backend compatibility.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to update documentation and ensure backend compatibility, which falls under the rule of not asking the author to ensure things or double-check. It doesn't provide a specific suggestion or point out a specific issue with the code.
8. src/lmnr/sdk/client/asynchronous/resources/agent.py:212
  • Draft comment:
    Handling of error chunk in non-streaming run—raising RuntimeError with chunk.root.error. Ensure backend error responses are correctly formatted.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is suggesting to ensure that backend error responses are correctly formatted. This falls under asking the PR author to ensure something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue with the code.
9. src/lmnr/sdk/client/synchronous/resources/agent.py:200
  • Draft comment:
    Synchronous agent now mirrors asynchronous version with new parameters and error handling. Confirm consistency in behavior.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to confirm consistency in behavior, which violates the rule against asking for confirmation or verification of behavior. It does not provide a specific code suggestion or ask for a specific test to be written.
10. src/lmnr/sdk/types.py:232
  • Draft comment:
    RunAgentRequest now supports additional fields (agent_state, storage_state, return_agent_state, return_storage_state, etc.) and the RunAgentResponseChunk now includes ErrorChunkContent. Ensure all consumers handle these fields appropriately.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is purely informative, providing details about new fields in a request and response. It does not suggest any specific action or ask for confirmation on any specific change. It violates the rule against making purely informative comments.
11. src/lmnr/version.py:6
  • Draft comment:
    Version bump updated to 0.5.2. Ensure release notes capture all introduced changes.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to ensure that release notes capture all introduced changes, which is similar to asking them to update the PR description or ensure something is documented. This violates the rule against asking the author to ensure behavior is intended or documented.
12. README.md:225
  • Draft comment:
    Update the API examples to use the new 'chunk_type' property consistently.
  • 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.
13. pyproject.toml:55
  • Draft comment:
    Version bumped to 0.5.2 and opentelemetry-instrumentation dependencies updated to 0.39.2. Verify these versions are compatible with the rest of the system.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is about dependency version changes and asks the author to verify compatibility. According to the rules, comments on dependency changes or asking the author to verify compatibility should be removed.
14. src/lmnr/__init__.py:7
  • Draft comment:
    Removed several unused/deprecated types (e.g., ChatMessage, NodeInput, PipelineRunError, PipelineRunResponse) – ensure that any external usage of these is updated.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that external usage of removed types is updated. This falls under the rule of not asking the author to ensure things are tested or verified, which is not allowed.
15. src/lmnr/openllmetry_sdk/__init__.py:73
  • Draft comment:
    The new shutdown method is a good addition; consider adding a guard (null-check) for __tracer_wrapper before calling shutdown.
  • 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 existing flush() method follows the same pattern without a null check. The class seems designed to have __tracer_wrapper initialized through init() before any other methods are called. Adding null checks would be inconsistent with the existing pattern. If __tracer_wrapper is null, it likely indicates a programming error where shutdown() was called before init(). The comment might have a point - what if someone calls shutdown() without calling init() first? That would cause an error. While that's true, the same applies to flush() which already exists. The class seems designed to require proper initialization, and adding defensive coding only to shutdown() would be inconsistent. The comment should be deleted. The suggested change would be inconsistent with the existing class design and patterns.
16. src/lmnr/sdk/client/asynchronous/async_client.py:66
  • Draft comment:
    Removal of pipeline and semantic_search resources is intentional. Double-check that no downstream code relies on these endpoints.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment asks the PR author to double-check that no downstream code relies on the removed endpoints. This violates the rule against asking the author to confirm or double-check things. The first part of the comment is purely informative, stating that the removal is intentional, which is also against the rules.
17. src/lmnr/sdk/client/asynchronous/resources/agent.py:38
  • Draft comment:
    New agent configuration parameters (agent_state, storage_state, return_agent_state, etc.) have been added. Ensure that the aliases in RunAgentRequest match backend expectations.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that the aliases in RunAgentRequest match backend expectations. This falls under the category of asking the author to ensure something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, making it not useful according to the guidelines.
18. src/lmnr/sdk/client/synchronous/resources/agent.py:148
  • Draft comment:
    Ensure consistency in parameter naming and alias conversion for synchronous run() (including returnAgentState, returnStorageState, etc.).
  • 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.
19. src/lmnr/sdk/laminar.py:652
  • Draft comment:
    Updated shutdown logic to call TracerManager.shutdown() instead of flush; this change is appropriate but verify that no necessary flush logic is omitted.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify that no necessary flush logic is omitted, which violates the rule against asking the author to ensure behavior is intended or to double-check things. The comment does not provide a specific suggestion or ask for a specific test to be written.
20. src/lmnr/sdk/types.py:315
  • Draft comment:
    The addition of ErrorChunkContent in the union for RunAgentResponseChunk improves error handling. Confirm that the backend returns the 'error' field consistently.
  • Reason this comment was not posted:
    Comment was on unchanged code.
21. src/lmnr/version.py:6
  • Draft comment:
    Version bump to 0.5.2 is reflected here; ensure this matches the release notes and other documentation.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to ensure that the version bump matches the release notes and other documentation. This falls under the category of asking the author to double-check things, which is against the rules.
22. src/lmnr/__init__.py:16
  • Draft comment:
    Typo in module name: 'openllmetry_sdk' appears to be misspelled. Perhaps it should be 'opentelemetry_sdk'?
  • 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.
23. src/lmnr/__init__.py:17
  • Draft comment:
    Typo in module name: 'openllmetry_sdk.tracing.attributes' seems to contain a misspelling. Verify if it should be 'opentelemetry_sdk.tracing.attributes'.
  • 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.
24. src/lmnr/sdk/laminar.py:240
  • Draft comment:
    There's an extra backtick at the end of the example code in the docstring for start_as_current_span. Please remove it for clarity.
  • 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.
25. src/lmnr/sdk/laminar.py:683
  • Draft comment:
    Typographical error in the set_metadata method docstring: 'Willl' should be corrected to 'will'.
  • 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_bG3ja1PhZIZ65vGl

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

Caution

Changes requested ❌

Reviewed 080cf69 in 1 minute and 29 seconds. Click for details.
  • Reviewed 39 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/sdk/types.py:327
  • Draft comment:
    Ensure that the new TimeoutChunkContent is consistently used across the codebase, and consider adding tests for timeout behavior.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. src/lmnr/sdk/client/asynchronous/resources/agent.py:299
  • Draft comment:
    Consider using a custom TimeoutError (or similar) instead of RuntimeError to better distinguish timeout errors.
  • Reason this comment was not posted:
    Marked as duplicate.
3. src/lmnr/sdk/client/synchronous/resources/agent.py:293
  • Draft comment:
    Consider using a custom TimeoutError (or similar) here instead of RuntimeError for clarity in timeout cases.
  • Reason this comment was not posted:
    Marked as duplicate.
4. src/lmnr/sdk/types.py:328
  • Draft comment:
    Ensure that downstream consumers of RunAgentResponseChunk are updated to handle TimeoutChunkContent appropriately.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to ensure that downstream consumers are updated, which is a form of asking them to double-check or verify something. This violates the rule against asking the author to ensure behavior is intended or tested.

Workflow ID: wflow_rxRQPly1blIDzaM2

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

dinmukhamedm and others added 2 commits April 21, 2025 19:13
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
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.

Caution

Changes requested ❌

Reviewed 3469b73 in 2 minutes and 37 seconds. Click for details.
  • Reviewed 788 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 15 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/openllmetry_sdk/decorators/base.py:42
  • Draft comment:
    Good integration of the new skip_input_keys parameter. Consider if using a more generic Iterable[str] might better support various input types.
  • 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% While using Iterable[str] would be more flexible and follow the collections.abc best practices, we don't see how the parameter is actually used in get_input_from_func_args(). Without seeing that implementation, we can't be sure if it specifically needs a list or if it would work with any iterable. The change could potentially introduce bugs if the receiving function expects list-specific operations. I don't have visibility into how get_input_from_func_args() uses this parameter internally. The receiving function might need list-specific functionality like indexing or mutability. While the suggestion follows good Python practices, without seeing the implementation details of get_input_from_func_args(), making this change could be risky. The comment should be removed since we don't have enough context to verify that this change would be safe and beneficial.
2. src/lmnr/sdk/decorators.py:26
  • Draft comment:
    skip_input_keys is correctly passed to both the synchronous and async wrappers. Ensure consistency in type annotations if you ever refactor.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. src/lmnr/sdk/utils.py:99
  • Draft comment:
    The check for skip_input_keys in get_input_from_func_args properly filters input keys. Consider adding edge-case tests if not already covered.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. tests/test_observe.py:174
  • Draft comment:
    New tests for skip_input_keys (sync and async) look well-constructed and cover the intended behavior.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
5. src/lmnr/openllmetry_sdk/decorators/base.py:42
  • Draft comment:
    Good integration of the new skip_input_keys parameter in both entity_method and aentity_method.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
6. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/__init__.py:47
  • Draft comment:
    Span names refer to 'Gemini'. Confirm naming consistency versus 'GoogleGenAi'.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
7. src/lmnr/sdk/utils.py:89
  • Draft comment:
    Using mutable defaults for func_args ([]) and func_kwargs ({}) may lead to unforeseen issues. Consider using None and initializing inside the function.
  • Reason this comment was not posted:
    Comment was on unchanged code.
8. tests/test_observe.py:174
  • Draft comment:
    Test for skip_input_keys works but verify that both positional and keyword arguments are handled, as keyword args in the skip list aren’t removed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
9. src/lmnr/openllmetry_sdk/decorators/base.py:1
  • Draft comment:
    It appears that 'openllmetry_sdk' might be a typo. Did you mean 'opentelemetry_sdk'? Please check the spelling in the folder and module names to ensure consistency with the upstream 'opentelemetry' naming.
  • 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.
10. src/lmnr/openllmetry_sdk/decorators/base.py:115
  • Draft comment:
    The async decorator function is named 'aentity_method'. If this naming is intentional to signify an asynchronous version of 'entity_method', it might be clearer to rename it to 'async_entity_method' to improve readability and avoid potential confusion.
  • Reason this comment was not posted:
    Comment was on unchanged code.
11. src/lmnr/openllmetry_sdk/instruments.py:1
  • Draft comment:
    It appears that 'openllmetry_sdk' might be a typographical error. Consider renaming it to 'opentelemetry_sdk' if this aligns with its intended purpose.
  • 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.
12. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/__init__.py:4
  • Draft comment:
    Typographical error: The URL in the comment uses 'openllmetry' (e.g., feat(google-genai): instrumentation for google-genai traceloop/openllmetry#2828). Should this be corrected to 'opentelemetry'?
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment appears to misunderstand that this is intentionally referencing the openllmetry repository, not opentelemetry. The URL is correct as-is since it's pointing to the actual PR that will be merged. Changing it would make it incorrect. This is a case where the automated tool didn't understand the context. Could there be a legitimate reason to change the URL? Maybe openllmetry is a typo or unofficial name that should be standardized? No, openllmetry appears to be the correct and intentional name of the repository where this PR exists. The comment is suggesting a change that would make the reference incorrect. Delete this comment as it suggests changing a correct URL to an incorrect one due to misunderstanding the context.
13. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/config.py:1
  • Draft comment:
    The directory name 'openllmetry_sdk' appears to have a typographical error and might be intended to be 'opentelemetry_sdk'. Please double-check and correct if necessary.
  • Reason this comment was not posted:
    Comment was on unchanged code.
14. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/utils.py:37
  • Draft comment:
    Typo in logging message: 'OpenLLMetry' may be a typo for 'OpenTelemetry'. Please review and correct the name if necessary.
  • 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% The file is part of a package called "openllmetry_sdk" which suggests this is an intentional name. The log message using "OpenLLMetry" is likely deliberate branding for this specific SDK, combining "LLM" and "OpenTelemetry". The automated tool incorrectly assumed this was a typo, but it appears to be intentional. I could be wrong about the branding intention - maybe it really is a typo. The package name could be a typo too. Given the consistent use in both the package name and the log message, and the clear combination of "LLM" + "Telemetry", this is very likely intentional branding rather than a typo. The comment should be deleted as it incorrectly identifies intentional branding as a typo.
15. src/lmnr/openllmetry_sdk/tracing/tracing.py:622
  • Draft comment:
    Typo: The error message in init_google_generativeai_instrumentor (line 622) refers to 'Gemini instrumentor' instead of 'Google GenerativeAI instrumentor'. Please update it for clarity.
  • 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_xusp0Q6YGoGg8VaU

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

dinmukhamedm and others added 4 commits April 21, 2025 19:16
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
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.

Caution

Changes requested ❌

Reviewed 45381f3 in 1 minute and 38 seconds. Click for details.
  • Reviewed 444 lines of code in 10 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/sdk/utils.py:95
  • Draft comment:
    Bug: 'skip_input_keys' is used in the dict comprehension but is not defined. It should be replaced with the 'ignore_inputs' parameter.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. src/lmnr/sdk/client/asynchronous/resources/agent.py:47
  • Draft comment:
    Enhancement: The new 'start_url' parameter is added to agent methods. Consider adding validation to ensure it is a well-formed URL to prevent runtime errors when interacting with Playwright.
  • 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% This is a client SDK that makes API calls to a backend. Adding URL validation here would be redundant since: 1. The backend service would need to validate URLs anyway for security 2. The docstring already points to Playwright docs for valid URL format 3. Adding validation here could lead to inconsistencies if backend validation changes 4. This is just a type definition/interface file with no implementation The comment does raise a valid point about preventing runtime errors early. Client-side validation could provide faster feedback to developers. While early validation can be helpful, in this case it would add complexity and potential maintenance burden for little benefit. The backend must validate URLs regardless, and the docstring already provides guidance. The comment should be deleted. URL validation belongs in the backend service, and the docstring already provides guidance on valid URL formats.
3. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/__init__.py:1
  • Draft comment:
    Typo Alert: The directory (and likely the package name) is spelled 'openllmetry_sdk' with an extra 'l'. It seems it should be 'opentelemetry_sdk' to align with the standard spelling of 'OpenTelemetry'. Please verify and adjust if necessary.
  • 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.
4. src/lmnr/openllmetry_sdk/tracing/tracing.py:622
  • Draft comment:
    Typo: In init_google_generativeai_instrumentor, the error message on line 622 reads "Error initializing Gemini instrumentor: {e}". It appears to be a copy-paste error. Consider updating it to a more accurate message (e.g., "Error initializing Google Generative AI instrumentor: {e}").
  • 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.
5. src/lmnr/sdk/decorators.py:1
  • Draft comment:
    It looks like the module is imported as 'lmnr.openllmetry_sdk', but typically 'opentelemetry' is spelled with a single 'l' ('opentelemetry'). Please double-check whether 'openllmetry_sdk' is an intentional naming or a typographical error.
  • 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.
6. src/lmnr/sdk/utils.py:95
  • Draft comment:
    Typo: In the dictionary comprehension initializing 'res', the variable 'skip_input_keys' is still used, but it should be 'ignore_inputs' as per the updated parameter name. Please update this for consistency.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_SQE0Soi7lVjr97ZU

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

) -> dict[str, typing.Any]:
# Remove implicitly passed "self" or "cls" argument for
# instance or class methods
res = func_kwargs.copy()
res = {k: v for k, v in func_kwargs.items() if not (skip_input_keys and k in skip_input_keys)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: 'skip_input_keys' is used instead of 'ignore_inputs' in the dict comprehension. Update to use 'ignore_inputs' to match the renamed parameter.

Suggested change
res = {k: v for k, v in func_kwargs.items() if not (skip_input_keys and k in skip_input_keys)}
res = {k: v for k, v in func_kwargs.items() if not (ignore_inputs and k in ignore_inputs)}

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.

Important

Looks good to me! 👍

Reviewed 8b1b6a9 in 54 seconds. Click for details.
  • Reviewed 54 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/sdk/utils.py:95
  • Draft comment:
    Rename 'skip_input_keys' to 'ignore_inputs' consistently. The change is correctly applied, but please confirm that the naming aligns with the rest of the codebase.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment suggests renaming a variable for consistency, which is a valid code suggestion. However, it also asks the author to confirm alignment with the rest of the codebase, which is not allowed. The useful part is the suggestion to rename the variable for consistency.
2. tests/test_observe.py:189
  • Draft comment:
    New test 'test_observe_skip_input_keys_with_kwargs' validates behavior when using ignore_inputs with kwargs. The test looks comprehensive.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, as it praises the addition of a new test without providing any actionable feedback or suggestions for improvement. It does not align with the rules for useful comments.
3. src/lmnr/sdk/utils.py:95
  • Draft comment:
    Renamed the filtering variable from 'skip_input_keys' to 'ignore_inputs'. Ensure that documentation and any related code references are updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that documentation and related code references are updated. This falls under the rule of not asking the author to ensure things are done, which is not allowed.
4. tests/test_observe.py:202
  • Draft comment:
    In the new test for kwargs filtering, consider comparing dictionaries directly instead of sorting, as Python dict equality is order-insensitive.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_pBsYJU98b7kEe7RP

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

Important

Looks good to me! 👍

Reviewed 809e15f in 1 minute and 27 seconds. Click for details.
  • Reviewed 19 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/test_observe.py:198
  • Draft comment:
    Removed sorting of dictionary keys is a cleaner approach; dict equality ignores order. Ensure that json.loads always returns a dict.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. tests/test_observe.py:198
  • Draft comment:
    Good improvement: removed unnecessary sorting & dict reconstruction. Directly comparing dictionaries is simpler and order-independent.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_fxlXb3x5dZiIW9ya

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

Caution

Changes requested ❌

Reviewed 0dd69a2 in 2 minutes and 9 seconds. Click for details.
  • Reviewed 285 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/utils.py:222
  • Draft comment:
    BUG: _run_async does not return the value from the async method. In _process_image_item the result of Config.upload_base64_image (expected to be a URL) is assigned to 'url' from _run_async, but _run_async always returns None. Consider capturing and returning the coroutine's result.
  • Reason this comment was not posted:
    Marked as duplicate.
2. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/__init__.py:1
  • Draft comment:
    It looks like the directory name 'openllmetry_sdk' might be a typographical error. Typically, the correct spelling is 'opentelemetry_sdk' to be consistent with the official OpenTelemetry naming. Please verify and correct if this is not intentional.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/config.py:1
  • Draft comment:
    The file is located under 'openllmetry_sdk', which appears to be a typo. If this is a mistake, consider renaming it to 'opentelemetry_sdk' to maintain consistency with the rest of the codebase.
  • Reason this comment was not posted:
    Comment was on unchanged code.
4. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/utils.py:37
  • Draft comment:
    Typo detected: The log message reads 'OpenLLMetry failed to trace in %s, error: %s'. If this is meant to refer to OpenTelemetry, please update it accordingly for clarity.
  • 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_OmhTCFrRV6vcJiDM

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

image_format = blob.get("mime_type").split("/")[1]
image_name = f"message_{message_index}_content_{content_index}.{image_format}"
base64_string = blob.get("data")
url = _run_async(
Copy link
Contributor

Choose a reason for hiding this comment

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

BUG: _run_async is expected to return the URL from the async upload, but it doesn't return any value. This causes _process_image_item to always get None for the URL.

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.

Important

Looks good to me! 👍

Reviewed 0171b4f in 1 minute and 1 seconds. Click for details.
  • Reviewed 40 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/utils.py:206
  • Draft comment:
    Removed async upload branch: _process_image_item now only returns a converted image URL if Config.convert_image_to_openai_format is true, else the raw blob. Ensure this removal of async uploading (via Config.upload_base64_image and _run_async) is intended.
  • 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.
2. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/utils.py:206
  • Draft comment:
    Removal of the upload branch: The async upload logic using Config.upload_base64_image and _run_async has been removed. Confirm that this change is intentional and that any related config and helper code (like _run_async) can be deprecated.
  • 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.
3. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/utils.py:199
  • Draft comment:
    Unused parameters: The parameters trace_id, span_id, message_index, and content_index are no longer used in _process_image_item. Consider removing them if they are not needed for API consistency.
  • 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.
4. src/lmnr/openllmetry_sdk/opentelemetry/instrumentation/google_genai/utils.py:37
  • Draft comment:
    Typographical error: The log message uses 'OpenLLMetry' which appears to be a typo. It is likely intended to be 'OpenTelemetry' to reflect the proper naming convention used elsewhere in the project.
  • 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_2OPKo7BSJRr8nFBU

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

Important

Looks good to me! 👍

Reviewed 0528f57 in 1 minute and 4 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/sdk/types.py:227
  • Draft comment:
    New enum members 'OPENAI' and 'GEMINI' were added. Ensure these align with existing naming conventions and that corresponding logic (e.g., in API integrations/tests/documentation) is updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 30% <= threshold 50% The comment is asking the author to ensure alignment with naming conventions and updates in related logic, which violates the rule against asking the author to ensure things. However, it does point out specific areas (API integrations/tests/documentation) that might need updates, which could be useful. Overall, it leans more towards being informative rather than actionable.
2. src/lmnr/sdk/types.py:227
  • Draft comment:
    New enum members OPENAI and GEMINI added. Ensure related code and docs are updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that related code and documentation are updated, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure things are updated or tested.

Workflow ID: wflow_bT3TvSYAdRimdX2C

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

Important

Looks good to me! 👍

Reviewed 53ff61d in 58 seconds. Click for details.
  • Reviewed 113 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. pyproject.toml:18
  • Draft comment:
    Added an upper bound for pydantic to prevent accidental upgrades to v3 which may introduce breaking changes.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining why an upper bound was added to a dependency. It doesn't provide a suggestion or ask for confirmation about a specific code change. According to the rules, purely informative comments should be removed.
2. src/lmnr/sdk/client/asynchronous/resources/agent.py:216
  • Draft comment:
    Renamed request parameter keys to snake_case to match the pydantic alias configuration.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining a change that was made without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
3. src/lmnr/sdk/client/synchronous/resources/agent.py:205
  • Draft comment:
    Updated RunAgentRequest keys to snake_case for consistency with the backend API.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining a change that was made. It does not provide any actionable feedback or suggestions for improvement.
4. src/lmnr/sdk/types.py:232
  • Draft comment:
    Enhanced RunAgentRequest configuration by adding 'populate_by_name=True' for proper alias resolution.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, describing a change made to the code without providing any actionable feedback or suggestions. It does not ask for confirmation or suggest improvements, nor does it highlight any potential issues or best practices.
5. pyproject.toml:18
  • Draft comment:
    Pinning pydantic to <3.0.0 is good for ensuring compatibility; consider noting why the upper bound is required.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to note why a specific version of a dependency is required. This falls under the rule of not commenting on dependency changes or asking for explanations, which is not allowed.
6. src/lmnr/sdk/client/asynchronous/resources/agent.py:214
  • Draft comment:
    Field names updated to snake_case; ensure the alias generator still produces the expected camelCase keys in the JSON output.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that a specific behavior is maintained after a change. This violates the rule against asking the author to ensure behavior is intended or tested. The comment does not provide a specific suggestion or ask for a specific test to be written.
7. src/lmnr/sdk/client/synchronous/resources/agent.py:205
  • Draft comment:
    Consistently updated field names to snake_case; verify that the returned JSON still matches backend expectations via the alias configuration.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify that the returned JSON matches backend expectations, which is a form of asking for confirmation or verification. This violates the rule against asking the PR author to confirm or ensure behavior. Therefore, this comment should be removed.
8. src/lmnr/sdk/types.py:232
  • Draft comment:
    Added 'populate_by_name=True' in RunAgentRequest's Config; this ensures models can be instantiated using both field names and aliases.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining what a code change does without suggesting any action or asking for clarification. It doesn't align with the rules for useful comments.

Workflow ID: wflow_BvUZAZTq8IWIV1oz

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@dinmukhamedm dinmukhamedm merged commit f18d2d3 into main Apr 22, 2025
7 checks passed
@dinmukhamedm dinmukhamedm deleted the error-chunk branch April 22, 2025 17:52
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