feat: Add Fireworks Python SDK support #592
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- Add FireworksProvider class implementing InstrumentedProvider - Support both synchronous and asynchronous completions - Handle streaming responses - Implement OpenAI-compatible interface Co-Authored-By: Alex Reibman <[email protected]>
- Add installation instructions for Fireworks SDK - Add environment variable setup guide - Include synchronous and asynchronous usage examples - Add streaming examples - Follow existing documentation structure Co-Authored-By: Alex Reibman <[email protected]>
- Add script to generate Jupyter notebook - Add example notebook with basic and streaming completions - Include environment variable setup - Add documentation and explanatory markdown cells Co-Authored-By: Alex Reibman <[email protected]>
…event tracking Co-Authored-By: Alex Reibman <[email protected]>
Co-Authored-By: Alex Reibman <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
⚙️ Control Options:
Add "(aside)" to your comment to have me ignore it. |
WalkthroughThis update integrates the Fireworks provider into the system, enabling both synchronous and asynchronous completions with streaming capabilities. The Changes
🔗 Related PRs
InstructionsEmoji Descriptions:
Interact with the Bot:
Execute a command using the format:
Available Commands:
Tips for Using @bot Effectively:
Need More Help?📚 Visit our documentation for detailed guides on using Entelligence.AI. |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
agentops/llms/providers/fireworks.py
Outdated
|
|
||
| def generator(stream): | ||
| for chunk in stream: | ||
| try: | ||
| # Parse the chunk data | ||
| if hasattr(chunk, 'choices') and chunk.choices: | ||
| content = chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, 'content') else None | ||
| else: | ||
| # Handle raw string chunks from streaming response | ||
| content = chunk | ||
|
|
||
| if content: | ||
| # Create event data for streaming chunk | ||
| event = LLMEvent( | ||
| event_type=EventType.LLM.value, | ||
| init_timestamp=init_timestamp, | ||
| end_timestamp=get_ISO_time(), | ||
| model=kwargs.get('model', 'unknown'), | ||
| prompt=str(kwargs.get('messages', [])), | ||
| completion="[Streaming Response]", | ||
| prompt_tokens=0, | ||
| completion_tokens=0, | ||
| cost=0.0 | ||
| ) | ||
| if self._session: | ||
| self._session.record(event) | ||
| logger.debug(f"Recorded streaming chunk for session {self._session.session_id}") | ||
| yield content | ||
| except Exception as e: | ||
| logger.error(f"Error processing streaming chunk: {str(e)}") | ||
| continue | ||
|
|
||
| if hasattr(response, '__aiter__'): | ||
| return async_generator(response) | ||
| else: | ||
| return generator(response) |
There was a problem hiding this comment.
🤖 Bug Fix:
Missing Asynchronous Generator for Streaming Responses
The current implementation attempts to handle streaming responses by checking for the __aiter__ attribute to decide whether to use an asynchronous generator. However, the async_generator function is not defined, which will lead to runtime errors when handling asynchronous responses. This is a critical issue as it can break the functionality of the FireworksProvider when dealing with asynchronous data streams.
Actionable Suggestions:
- Define the
async_generatorfunction to properly handle asynchronous streaming responses. This function should mirror the logic of the synchronousgeneratorfunction but should be capable of handling asynchronous iteration. - Ensure that the
async_generatorfunction correctly yields content from the asynchronous response, handling any exceptions that may occur during iteration.
By implementing these changes, the code will be robust against both synchronous and asynchronous streaming responses, ensuring consistent functionality across different response types.
🔧 Suggested Code Diff:
async def async_generator(stream):
async for chunk in stream:
try:
# Parse the chunk data
if hasattr(chunk, 'choices') and chunk.choices:
content = chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, 'content') else None
else:
# Handle raw string chunks from streaming response
content = chunk
if content:
# Create event data for streaming chunk
event = LLMEvent(
event_type=EventType.LLM.value,
init_timestamp=init_timestamp,
end_timestamp=get_ISO_time(),
model=kwargs.get('model', 'unknown'),
prompt=str(kwargs.get('messages', [])),
completion="[Streaming Response]",
prompt_tokens=0,
completion_tokens=0,
cost=0.0
)
if self._session:
self._session.record(event)
logger.debug(f"Recorded streaming chunk for session {self._session.session_id}")
yield content
except Exception as e:
logger.error(f"Error processing streaming chunk: {str(e)}")
continue📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def generator(stream): | |
| for chunk in stream: | |
| try: | |
| # Parse the chunk data | |
| if hasattr(chunk, 'choices') and chunk.choices: | |
| content = chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, 'content') else None | |
| else: | |
| # Handle raw string chunks from streaming response | |
| content = chunk | |
| if content: | |
| # Create event data for streaming chunk | |
| event = LLMEvent( | |
| event_type=EventType.LLM.value, | |
| init_timestamp=init_timestamp, | |
| end_timestamp=get_ISO_time(), | |
| model=kwargs.get('model', 'unknown'), | |
| prompt=str(kwargs.get('messages', [])), | |
| completion="[Streaming Response]", | |
| prompt_tokens=0, | |
| completion_tokens=0, | |
| cost=0.0 | |
| ) | |
| if self._session: | |
| self._session.record(event) | |
| logger.debug(f"Recorded streaming chunk for session {self._session.session_id}") | |
| yield content | |
| except Exception as e: | |
| logger.error(f"Error processing streaming chunk: {str(e)}") | |
| continue | |
| if hasattr(response, '__aiter__'): | |
| return async_generator(response) | |
| else: | |
| return generator(response) | |
| async def async_generator(stream): | |
| async for chunk in stream: | |
| try: | |
| # Parse the chunk data | |
| if hasattr(chunk, 'choices') and chunk.choices: | |
| content = chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, 'content') else None | |
| else: | |
| # Handle raw string chunks from streaming response | |
| content = chunk | |
| if content: | |
| # Create event data for streaming chunk | |
| event = LLMEvent( | |
| event_type=EventType.LLM.value, | |
| init_timestamp=init_timestamp, | |
| end_timestamp=get_ISO_time(), | |
| model=kwargs.get('model', 'unknown'), | |
| prompt=str(kwargs.get('messages', [])), | |
| completion="[Streaming Response]", | |
| prompt_tokens=0, | |
| completion_tokens=0, | |
| cost=0.0 | |
| ) | |
| if self._session: | |
| self._session.record(event) | |
| logger.debug(f"Recorded streaming chunk for session {self._session.session_id}") | |
| yield content | |
| except Exception as e: | |
| logger.error(f"Error processing streaming chunk: {str(e)}") | |
| continue | |
| def generator(stream): | |
| for chunk in stream: | |
| try: | |
| # Parse the chunk data | |
| if hasattr(chunk, 'choices') and chunk.choices: | |
| content = chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, 'content') else None | |
| else: | |
| # Handle raw string chunks from streaming response | |
| content = chunk | |
| if content: | |
| # Create event data for streaming chunk | |
| event = LLMEvent( | |
| event_type=EventType.LLM.value, | |
| init_timestamp=init_timestamp, | |
| end_timestamp=get_ISO_time(), | |
| model=kwargs.get('model', 'unknown'), | |
| prompt=str(kwargs.get('messages', [])), | |
| completion="[Streaming Response]", | |
| prompt_tokens=0, | |
| completion_tokens=0, | |
| cost=0.0 | |
| ) | |
| if self._session: | |
| self._session.record(event) | |
| logger.debug(f"Recorded streaming chunk for session {self._session.session_id}") | |
| yield content | |
| except Exception as e: | |
| logger.error(f"Error processing streaming chunk: {str(e)}") | |
| continue | |
| if hasattr(response, '__aiter__'): | |
| return async_generator(response) | |
| else: | |
| return generator(response) |
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Follow PEP 8 style guide for consistent code formatting
| else: | ||
| logger.warning(f"Only AI21>=2.0.0 supported. v{module_version} found.") | ||
|
|
||
| if api == "fireworks-ai": | ||
| module_version = version(api) | ||
|
|
||
| if Version(module_version) >= parse("0.1.0"): | ||
| provider = FireworksProvider(self.client) | ||
| provider.override() | ||
| else: | ||
| logger.warning(f"Only Fireworks>=0.1.0 supported. v{module_version} found.") | ||
|
|
||
| if api == "llama_stack_client": | ||
| module_version = version(api) | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Enhance Error Handling for Unsupported FireworksProvider Versions
The current implementation logs a warning when the FireworksProvider version is below 0.1.0, but it does not prevent the system from attempting to use an unsupported provider. This could lead to unexpected behavior or errors later in the execution. To address this, it's recommended to implement a fallback mechanism or raise an exception to ensure the system does not proceed with an unsupported version. This will improve the robustness and reliability of the code.
Actionable Steps:
- Implement a fallback mechanism that either uses a default provider or skips the operation if the version is unsupported.
- Alternatively, raise an exception to halt execution and notify the user of the unsupported version.
- Ensure that any changes are accompanied by appropriate unit tests to verify the new behavior.
🔧 Suggested Code Diff:
else:
logger.warning(f"Only Fireworks>=0.1.0 supported. v{module_version} found.")
+ raise RuntimeError(f"Unsupported Fireworks version: {module_version}. Please upgrade to >=0.1.0.")📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| else: | |
| logger.warning(f"Only AI21>=2.0.0 supported. v{module_version} found.") | |
| if api == "fireworks-ai": | |
| module_version = version(api) | |
| if Version(module_version) >= parse("0.1.0"): | |
| provider = FireworksProvider(self.client) | |
| provider.override() | |
| else: | |
| logger.warning(f"Only Fireworks>=0.1.0 supported. v{module_version} found.") | |
| if api == "llama_stack_client": | |
| module_version = version(api) | |
| else: | |
| logger.warning(f"Only AI21>=2.0.0 supported. v{module_version} found.") | |
| if api == "fireworks-ai": | |
| module_version = version(api) | |
| if Version(module_version) >= parse("0.1.0"): | |
| provider = FireworksProvider(self.client) | |
| provider.override() | |
| else: | |
| logger.warning(f"Only Fireworks>=0.1.0 supported. v{module_version} found.") | |
| raise RuntimeError(f"Unsupported Fireworks version: {module_version}. Please upgrade to >=0.1.0.") | |
| if api == "llama_stack_client": | |
| module_version = version(api) |
📜 Guidelines
• Use exceptions for error handling rather than return codes
Codecov ReportAttention: Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Co-Authored-By: Alex Reibman <[email protected]>
Co-Authored-By: Alex Reibman <[email protected]>
Co-Authored-By: Alex Reibman <[email protected]>
Co-Authored-By: Alex Reibman <[email protected]>
agentops/llms/providers/fireworks.py
Outdated
|
|
||
| async def async_generator(stream): | ||
| self._accumulated_content = "" | ||
| async for chunk in stream: | ||
| try: | ||
| if hasattr(chunk, "choices") and chunk.choices: | ||
| content = ( | ||
| chunk.choices[0].delta.content | ||
| if hasattr(chunk.choices[0].delta, "content") | ||
| else None | ||
| ) | ||
| else: | ||
| content = chunk | ||
|
|
||
| if content: | ||
| self._accumulated_content += content | ||
| # Record event only when we have the complete response | ||
| if hasattr(chunk.choices[0], "finish_reason") and chunk.choices[0].finish_reason: | ||
| stream_event.completion = self._accumulated_content | ||
| stream_event.end_timestamp = get_ISO_time() | ||
| if self._session: | ||
| self._session.record(stream_event) | ||
| logger.debug(f"Recorded streaming response for session {self._session.session_id}") | ||
| yield content |
There was a problem hiding this comment.
🤖 Bug Fix:
Ensure Safe Access to List Elements in Streaming Response
The current implementation attempts to access chunk.choices[0] without verifying if chunk.choices is a non-empty list. This can lead to an IndexError if chunk.choices is empty, potentially disrupting the application's functionality. To prevent this, it's crucial to add a check to ensure chunk.choices is not only present but also contains elements before accessing its first element. This will enhance the robustness of the code and prevent runtime errors.
🔧 Suggested Code Diff:
async def async_generator(stream):
self._accumulated_content = ""
async for chunk in stream:
try:
- if hasattr(chunk, "choices") and chunk.choices:
+ if hasattr(chunk, "choices") and chunk.choices and len(chunk.choices) > 0:
content = (
chunk.choices[0].delta.content
if hasattr(chunk.choices[0].delta, "content")
else None
)
else:
content = chunk
if content:
self._accumulated_content += content
# Record event only when we have the complete response
if hasattr(chunk.choices[0], "finish_reason") and chunk.choices[0].finish_reason:
stream_event.completion = self._accumulated_content
stream_event.end_timestamp = get_ISO_time()
if self._session:
self._session.record(stream_event)
logger.debug(f"Recorded streaming response for session {self._session.session_id}")
yield content
except Exception as e:
logger.error(f"Error processing streaming chunk: {str(e)}")
continue📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| async def async_generator(stream): | |
| self._accumulated_content = "" | |
| async for chunk in stream: | |
| try: | |
| if hasattr(chunk, "choices") and chunk.choices: | |
| content = ( | |
| chunk.choices[0].delta.content | |
| if hasattr(chunk.choices[0].delta, "content") | |
| else None | |
| ) | |
| else: | |
| content = chunk | |
| if content: | |
| self._accumulated_content += content | |
| # Record event only when we have the complete response | |
| if hasattr(chunk.choices[0], "finish_reason") and chunk.choices[0].finish_reason: | |
| stream_event.completion = self._accumulated_content | |
| stream_event.end_timestamp = get_ISO_time() | |
| if self._session: | |
| self._session.record(stream_event) | |
| logger.debug(f"Recorded streaming response for session {self._session.session_id}") | |
| yield content | |
| async def async_generator(stream): | |
| self._accumulated_content = "" | |
| async for chunk in stream: | |
| try: | |
| if hasattr(chunk, "choices") and chunk.choices and len(chunk.choices) > 0: | |
| content = ( | |
| chunk.choices[0].delta.content | |
| if hasattr(chunk.choices[0].delta, "content") | |
| else None | |
| ) | |
| else: | |
| content = chunk | |
| if content: | |
| self._accumulated_content += content | |
| # Record event only when we have the complete response | |
| if hasattr(chunk.choices[0], "finish_reason") and chunk.choices[0].finish_reason: | |
| stream_event.completion = self._accumulated_content | |
| stream_event.end_timestamp = get_ISO_time() | |
| if self._session: | |
| self._session.record(stream_event) | |
| logger.debug(f"Recorded streaming response for session {self._session.session_id}") | |
| yield content | |
| except Exception as e: | |
| logger.error(f"Error processing streaming chunk: {str(e)}") | |
| continue |
📜 Guidelines
• Use exceptions for error handling, but avoid assert statements for critical logic
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
Ensure Compatibility with New Return Type
The change in the return type of the end_session method from Union[Decimal, None] to Union[Dict[str, Any], None] is significant and can lead to logical errors if not handled properly. This change requires a thorough review of all instances where this method is called to ensure that the new return type is compatible with existing logic.
Actionable Steps:
- Audit Codebase: Identify all usages of the
end_sessionmethod. This includes direct calls and any indirect dependencies. - Update Logic: Modify the logic that processes the return value to handle a dictionary instead of a decimal. This might involve changing how the return value is accessed and used.
- Add Type Checks: Implement type checks or assertions where necessary to ensure that the return value is being used correctly.
- Testing: Conduct thorough testing to ensure that the changes do not introduce new bugs or regressions.
By following these steps, you can mitigate the risk of runtime errors and ensure that the codebase remains robust and maintainable. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| "blue", | ||
| ) | ||
| ) | ||
| return self.token_cost | ||
| return analytics_stats | ||
|
|
||
| def add_tags(self, tags: List[str]) -> None: | ||
| """ |
There was a problem hiding this comment.
🤖 Bug Fix:
Ensure Consistency in Return Value of end_session Method
The change in the return value of the end_session method from self.token_cost to analytics_stats is significant and could lead to logical errors if not handled properly. Here are the steps to address this:
- Verify Definition: Ensure that
analytics_statsis correctly defined and initialized with all necessary session statistics. This will prevent runtime errors and ensure that the method returns meaningful data. - Update Dependent Code: Review all parts of the codebase that call
end_sessionto ensure they are updated to handle the new return type and structure. This includes checking for any assumptions about the return value that might no longer hold true. - Testing: Add comprehensive tests to validate the new functionality. This should include unit tests to check the correctness of
analytics_statsand integration tests to ensure that the change does not introduce regressions in the system.
By following these steps, you can mitigate the risk of introducing logical errors and ensure that the system remains robust and reliable. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
Co-Authored-By: Alex Reibman <[email protected]>
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Change in Return Type from Decimal to Dict
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is significant and can lead to logical errors if not handled properly. This change requires a thorough review of all the places where this function is called to ensure that the new return type is managed correctly.
Actionable Steps:
- Audit Call Sites: Review all instances where this function is called to ensure they handle the new dictionary return type. This includes updating any logic that previously expected a
Decimal. - Update Documentation: Ensure that the function's documentation reflects the new return type and provides guidance on how to handle it.
- Testing: Implement unit tests to verify that the function behaves as expected with the new return type and that all calling code is updated accordingly.
This change is critical and should be prioritized to prevent runtime errors and ensure system stability. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
agentops/llms/providers/fireworks.py
Outdated
| async def async_generator(stream): | ||
| """Handle async streaming response.""" | ||
| self._accumulated_content = "" | ||
| async for chunk in stream: | ||
| try: | ||
| if hasattr(chunk, "choices") and chunk.choices: | ||
| content = ( | ||
| chunk.choices[0].delta.content | ||
| if hasattr(chunk.choices[0].delta, "content") | ||
| else None | ||
| ) | ||
| else: | ||
| content = chunk | ||
|
|
||
| if content: | ||
| self._accumulated_content += content | ||
| # Only create event when stream is finished | ||
| if hasattr(chunk.choices[0], "finish_reason") and chunk.choices[0].finish_reason: | ||
| stream_event.completion = self._accumulated_content | ||
| stream_event.end_timestamp = get_ISO_time() | ||
| if self._session: | ||
| self._session.record(stream_event) | ||
| logger.debug(f"Recorded complete streaming response for session {self._session.session_id}") | ||
| self._accumulated_content = "" # Reset for next stream | ||
| yield content |
There was a problem hiding this comment.
🤖 Bug Fix:
Ensure Safe Access to Streaming Response Attributes
The current implementation assumes that chunk.choices[0] always exists and has a finish_reason attribute. This assumption can lead to an AttributeError if chunk.choices is empty or if chunk.choices[0] does not have the expected attributes. To prevent this, it's crucial to add checks to ensure that chunk.choices is not empty and that chunk.choices[0] has the finish_reason attribute before accessing them. This will ensure that the function handles streaming responses robustly and avoids unexpected crashes.
🔧 Suggested Code Diff:
if hasattr(chunk, 'choices') and chunk.choices and hasattr(chunk.choices[0], 'finish_reason'):
if chunk.choices[0].finish_reason:
stream_event.completion = self._accumulated_content
stream_event.end_timestamp = get_ISO_time()📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| async def async_generator(stream): | |
| """Handle async streaming response.""" | |
| self._accumulated_content = "" | |
| async for chunk in stream: | |
| try: | |
| if hasattr(chunk, "choices") and chunk.choices: | |
| content = ( | |
| chunk.choices[0].delta.content | |
| if hasattr(chunk.choices[0].delta, "content") | |
| else None | |
| ) | |
| else: | |
| content = chunk | |
| if content: | |
| self._accumulated_content += content | |
| # Only create event when stream is finished | |
| if hasattr(chunk.choices[0], "finish_reason") and chunk.choices[0].finish_reason: | |
| stream_event.completion = self._accumulated_content | |
| stream_event.end_timestamp = get_ISO_time() | |
| if self._session: | |
| self._session.record(stream_event) | |
| logger.debug(f"Recorded complete streaming response for session {self._session.session_id}") | |
| self._accumulated_content = "" # Reset for next stream | |
| yield content | |
| async def async_generator(stream): | |
| """Handle async streaming response.""" | |
| self._accumulated_content = "" | |
| async for chunk in stream: | |
| try: | |
| if hasattr(chunk, "choices") and chunk.choices: | |
| choice = chunk.choices[0] | |
| content = ( | |
| choice.delta.content | |
| if hasattr(choice.delta, "content") | |
| else None | |
| ) | |
| else: | |
| content = chunk | |
| if content: | |
| self._accumulated_content += content | |
| # Only create event when stream is finished | |
| if hasattr(choice, "finish_reason") and choice.finish_reason: | |
| stream_event.completion = self._accumulated_content | |
| stream_event.end_timestamp = get_ISO_time() | |
| except AttributeError as e: | |
| # Log the error or handle it appropriately | |
| logger.error(f"Attribute error in async_generator: {e}") | |
| continue |
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Review of Return Type Change in Function
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is significant and could potentially break existing code that relies on the previous return type. This change should be carefully reviewed and tested. Here are the steps to address this:
- Review Usage: Identify all instances where this function is called and ensure that the calling code can handle a
Dict[str, Any]return type instead of aDecimal. - Update Dependent Code: Modify any dependent code to correctly process the dictionary structure. This might involve updating how the return value is accessed and used.
- Documentation: Update the function's documentation to reflect the new return type and provide examples of how to handle it.
- Testing: Implement comprehensive tests to cover scenarios with the new return type. Ensure that edge cases are considered to prevent runtime errors.
This change is critical and should be handled with caution to avoid introducing bugs into the system. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
Co-Authored-By: Alex Reibman <[email protected]>
Co-Authored-By: Alex Reibman <[email protected]>
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Change in Return Type from Decimal to Dict
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is significant and can potentially break existing code that relies on the previous return type. This change should be carefully reviewed and tested to ensure compatibility across the codebase.
Actionable Steps:
- Review Usage: Identify all instances where this function is called and ensure that the calling code can handle a
Dict[str, Any]instead of aDecimal. - Update Documentation: Modify the function's docstring to reflect the new return type and provide examples of the expected dictionary structure.
- Testing: Update existing tests to accommodate the new return type and add new tests if necessary to cover edge cases.
- Communicate Changes: Inform the team about this change to ensure that everyone is aware and can make necessary adjustments in their respective modules.
This change is critical and should be handled with caution to avoid runtime errors or unexpected behavior in the application.
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Use docstrings for modules, functions, and classes
• Write unit tests for your code
agentops/llms/providers/fireworks.py
Outdated
| def generator(stream_response): | ||
| accumulated_content = "" | ||
| try: | ||
| for chunk in stream_response: | ||
| if hasattr(chunk, "choices") and chunk.choices: | ||
| content = chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, "content") else "" | ||
| if content: | ||
| accumulated_content += content | ||
| yield chunk | ||
| # Update event with final accumulated content | ||
| event.completion = accumulated_content | ||
| event.end_timestamp = time.time() | ||
| if current_session: | ||
| current_session.record_event(event) | ||
| logger.info("Recorded streaming LLM event") | ||
| except Exception as e: | ||
| logger.error(f"Error in generator: {str(e)}") | ||
| raise | ||
|
|
||
| if hasattr(response, "__aiter__"): | ||
| return async_generator(response) # Return async generator | ||
| else: | ||
| return generator(response) # Return sync generator |
There was a problem hiding this comment.
🤖 Bug Fix:
Ensure Safe Access to 'chunk.choices' in Generators
The current implementation of the generator and async_generator functions does not adequately handle cases where chunk.choices might be None or not iterable. This could lead to an AttributeError, causing the application to crash unexpectedly. To resolve this, it's crucial to add a check to ensure chunk.choices is not None and is iterable before accessing chunk.choices[0].delta.content. This will enhance the robustness of the code and prevent potential runtime errors.
🔧 Suggested Code Diff:
def generator(stream_response):
accumulated_content = ""
try:
for chunk in stream_response:
+ if chunk.choices and hasattr(chunk.choices[0].delta, "content"):
content = chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, "content") else ""
if content:
accumulated_content += content
yield chunk
# Update event with final accumulated content
event.completion = accumulated_content
event.end_timestamp = time.time()
if current_session:
current_session.record_event(event)
logger.info("Recorded streaming LLM event")
except Exception as e:
logger.error(f"Error in generator: {str(e)}")
raise📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def generator(stream_response): | |
| accumulated_content = "" | |
| try: | |
| for chunk in stream_response: | |
| if hasattr(chunk, "choices") and chunk.choices: | |
| content = chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, "content") else "" | |
| if content: | |
| accumulated_content += content | |
| yield chunk | |
| # Update event with final accumulated content | |
| event.completion = accumulated_content | |
| event.end_timestamp = time.time() | |
| if current_session: | |
| current_session.record_event(event) | |
| logger.info("Recorded streaming LLM event") | |
| except Exception as e: | |
| logger.error(f"Error in generator: {str(e)}") | |
| raise | |
| if hasattr(response, "__aiter__"): | |
| return async_generator(response) # Return async generator | |
| else: | |
| return generator(response) # Return sync generator | |
| def generator(stream_response): | |
| accumulated_content = "" | |
| try: | |
| for chunk in stream_response: | |
| if chunk.choices and isinstance(chunk.choices, list) and hasattr(chunk.choices[0].delta, "content"): | |
| content = chunk.choices[0].delta.content | |
| if content: | |
| accumulated_content += content | |
| yield chunk | |
| # Update event with final accumulated content | |
| event.completion = accumulated_content | |
| event.end_timestamp = time.time() | |
| if current_session: | |
| current_session.record_event(event) | |
| logger.info("Recorded streaming LLM event") | |
| except Exception as e: | |
| logger.error(f"Error in generator: {str(e)}") | |
| raise | |
| if hasattr(response, "__aiter__"): | |
| return async_generator(response) # Return async generator | |
| else: | |
| return generator(response) # Return sync generator |
📜 Guidelines
• Use exceptions for error handling, but avoid assert statements for critical logic
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major modification that can lead to runtime errors if not handled properly by the consumers of this function. This change can break existing functionality, especially if the consumers are expecting a Decimal and are not prepared to handle a Dict.
Actionable Steps:
- Review Consumers: Thoroughly review all the consumers of this function to ensure they are updated to handle the new return type. This includes checking for any type checks or operations that assume a
Decimal. - Update Documentation: If this change is intentional, update the function's documentation to reflect the new return type and its structure.
- Migration Guide: If this function is part of a public API, consider providing a migration guide to assist users in adapting to the change.
- Testing: Ensure that comprehensive tests are in place to verify that the function behaves as expected with the new return type.
This change should be communicated clearly to all stakeholders to prevent any unexpected issues in production environments. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a major modification that can have widespread implications on the codebase. This change can lead to runtime errors or incorrect behavior if the calling code is not updated to handle the new return type.
Actionable Steps:
- Review Usage: Thoroughly review all instances where this function is called to ensure they are compatible with the new return type.
- Update Dependent Code: Modify any code that processes the return value to handle a dictionary instead of a decimal.
- Documentation: Update the function's documentation to reflect the new return type and provide examples of how to handle it.
- Migration Guide: Consider creating a migration guide to assist other developers in adapting to this change.
This change should be communicated clearly to all team members to prevent any unexpected issues in the system. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Return Type Requires Thorough Review
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a critical modification that can have widespread implications on the codebase. This change could potentially break existing functionality if the dependent code is not updated to handle the new return type correctly.
Actionable Steps:
- Review Usage: Conduct a comprehensive review of all instances where this function is called to ensure compatibility with the new return type.
- Update Dependent Code: Modify any code that relies on the previous return type to handle a dictionary return type appropriately.
- Testing: Implement thorough testing to ensure that the change does not introduce any bugs or unexpected behavior.
- Documentation: Update the function's documentation to reflect the new return type and inform all relevant stakeholders about this change.
This change is high-impact and requires careful consideration and communication to avoid disruptions. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
• Use meaningful variable and function names following specific naming conventions
Co-Authored-By: Alex Reibman <[email protected]>
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant API Contract Change Detected
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a major alteration in the API contract. This can potentially break existing code that relies on the previous return type.
Actionable Steps:
- Review Usage: Thoroughly review all instances where this function is used in the codebase to ensure compatibility with the new return type.
- Update Dependent Code: Modify any dependent code to handle the new return type appropriately. This may involve changes in how the return value is processed or stored.
- Documentation: If this function is part of a public API, update the documentation to reflect the change. Consider providing a migration guide to assist users in adapting to the new return type.
- Testing: Implement comprehensive tests to ensure that the new return type is handled correctly across the application.
This change is critical and should be handled with care to avoid introducing bugs or breaking existing functionality. 🛠️
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a major modification that can potentially break existing code that depends on this function. This change requires a thorough review of all instances where this function is used to ensure compatibility with the new return type.
Actionable Steps:
- Review Usage: Identify all places in the codebase where this function is called and verify that the new return type is handled correctly.
- Update Dependent Code: Modify any dependent code to accommodate the new return type. This may involve changes in how the return value is processed or stored.
- Documentation: Update the function's documentation to reflect the new return type and provide examples of how to handle it.
- Testing: Implement comprehensive tests to ensure that the function behaves as expected with the new return type and that no existing functionality is broken.
This change is critical and should be approached with caution to avoid introducing bugs into the system. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
agentops/llms/providers/fireworks.py
Outdated
| def _override_fireworks_completion(self): | ||
| """Override synchronous completion method.""" | ||
| original_create = self._original_completion | ||
| provider = self | ||
|
|
||
| def patched_function(*args, **kwargs): | ||
| try: | ||
| init_timestamp = time.time() | ||
| response = original_create(*args, **kwargs) | ||
| if kwargs.get("stream", False): | ||
| return provider.handle_response(response, kwargs, init_timestamp, provider._session) | ||
| else: | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
| return loop.run_until_complete( | ||
| provider.handle_response(response, kwargs, init_timestamp, provider._session) | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Error in Fireworks completion: {str(e)}") | ||
| raise | ||
|
|
||
| self.client.chat.completions.create = patched_function | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Avoid Creating New Event Loops in Asynchronous Code
The current implementation creates a new event loop for handling non-streaming responses, which can lead to runtime errors or unexpected behavior in environments where an event loop is already running. This is a critical issue, especially in asynchronous applications, as it can break functionality.
Actionable Suggestions:
- Use
asyncio.get_event_loop()to check if an existing event loop is running and use it. - Only create a new event loop if none is running. This approach ensures compatibility with environments that already have an event loop in place.
By following these steps, you can prevent potential issues related to event loop management and ensure the code functions correctly in various environments.
🔧 Suggested Code Diff:
def patched_function(*args, **kwargs):
try:
init_timestamp = time.time()
response = original_create(*args, **kwargs)
if kwargs.get("stream", False):
return provider.handle_response(response, kwargs, init_timestamp, provider._session)
else:
- loop = asyncio.new_event_loop()
- asyncio.set_event_loop(loop)
+ loop = asyncio.get_event_loop()
+ if loop.is_running():
+ return loop.run_until_complete(
+ provider.handle_response(response, kwargs, init_timestamp, provider._session)
+ )
+ else:
+ new_loop = asyncio.new_event_loop()
+ asyncio.set_event_loop(new_loop)
+ return new_loop.run_until_complete(
+ provider.handle_response(response, kwargs, init_timestamp, provider._session)
+ )
except Exception as e:
logger.error(f"Error in Fireworks completion: {str(e)}")
raise📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def _override_fireworks_completion(self): | |
| """Override synchronous completion method.""" | |
| original_create = self._original_completion | |
| provider = self | |
| def patched_function(*args, **kwargs): | |
| try: | |
| init_timestamp = time.time() | |
| response = original_create(*args, **kwargs) | |
| if kwargs.get("stream", False): | |
| return provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| else: | |
| loop = asyncio.new_event_loop() | |
| asyncio.set_event_loop(loop) | |
| return loop.run_until_complete( | |
| provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| ) | |
| except Exception as e: | |
| logger.error(f"Error in Fireworks completion: {str(e)}") | |
| raise | |
| self.client.chat.completions.create = patched_function | |
| def _override_fireworks_completion(self): | |
| """Override synchronous completion method.""" | |
| original_create = self._original_completion | |
| provider = self | |
| def patched_function(*args, **kwargs): | |
| try: | |
| init_timestamp = time.time() | |
| response = original_create(*args, **kwargs) | |
| if kwargs.get("stream", False): | |
| return provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| else: | |
| loop = asyncio.get_event_loop() | |
| if loop.is_running(): | |
| return loop.run_until_complete( | |
| provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| ) | |
| else: | |
| new_loop = asyncio.new_event_loop() | |
| asyncio.set_event_loop(new_loop) | |
| return new_loop.run_until_complete( | |
| provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| ) | |
| except Exception as e: | |
| logger.error(f"Error in Fireworks completion: {str(e)}") | |
| raise | |
| self.client.chat.completions.create = patched_function |
📜 Guidelines
• Use exceptions for error handling, but avoid assert statements for critical logic
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major alteration that can have widespread implications across the codebase. This change could potentially break existing functionality that relies on the previous return type.
Actionable Steps:
- Review Function Logic: Ensure that the function's logic aligns with the new return type. Verify that the function indeed returns a
Dict[str, Any]in all scenarios where it previously returned aDecimal. - Update Dependent Code: Identify all parts of the codebase that interact with this function and update them to handle the new return type appropriately.
- Documentation and Type Hints: Update any related documentation and type hints to reflect this change, ensuring that all developers are aware of the new expected output.
- Testing: Implement comprehensive tests to cover the new return type scenarios, ensuring that the function behaves as expected in all cases.
This change is critical and should be handled with care to avoid introducing bugs. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a major modification that can impact any code that depends on this function. This change requires a thorough review of all instances where this function is used to ensure compatibility with the new return type.
Actionable Steps:
- Audit Usage: Identify all places in the codebase where this function is called.
- Update Dependent Code: Modify the calling code to handle the new return type,
Dict[str, Any]. This may involve updating how the return value is processed, stored, or passed to other functions. - Testing: Implement comprehensive tests to ensure that the changes do not introduce any regressions or new bugs.
- Documentation: Update any relevant documentation to reflect the change in the return type and its implications.
This change is critical and should be handled with care to avoid breaking existing functionality. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| is_auto_end (bool, optional): is this an automatic use of end_session and should be skipped with skip_auto_end_session | ||
|
|
||
| Returns: | ||
| Decimal: The token cost of the session. Returns 0 if the cost is unknown. | ||
| Dict[str, Any]: Session statistics including duration, cost, and event counts. | ||
| """ | ||
| session = self._safe_get_session() | ||
| if session is None: | ||
| return | ||
| return None | ||
| if is_auto_end and self._config.skip_auto_end_session: | ||
| return | ||
|
|
||
| token_cost = session.end_session(end_state=end_state, end_state_reason=end_state_reason, video=video) | ||
| return None | ||
|
|
||
| return token_cost | ||
| return session.end_session(end_state=end_state, end_state_reason=end_state_reason, video=video) | ||
|
|
||
| def create_agent( | ||
| self, |
There was a problem hiding this comment.
🤖 Bug Fix:
Change in Return Type from Decimal to Dict
The change in the return type from Decimal to Dict[str, Any] is significant and can potentially break existing code that relies on the previous return type. This change should be carefully reviewed to ensure that all parts of the codebase that use this function are updated to handle the new return type correctly.
Actionable Steps:
- Audit Usage: Review all instances where this function is called to ensure they are compatible with the new return type.
- Update Dependent Code: Modify any dependent code to handle the dictionary structure, extracting necessary information such as duration, cost, and event counts.
- Testing: Implement comprehensive tests to verify that the changes do not introduce any regressions or unexpected behavior.
This change is critical and should be handled with caution to avoid breaking existing functionality. 🛠️
agentops/llms/providers/fireworks.py
Outdated
| def _override_fireworks_completion(self): | ||
| """Override synchronous completion method.""" | ||
| original_create = self._original_completion | ||
| provider = self | ||
|
|
||
| def patched_function(*args, **kwargs): | ||
| try: | ||
| init_timestamp = time.time() | ||
| response = original_create(*args, **kwargs) | ||
| if kwargs.get("stream", False): | ||
| return provider.handle_response(response, kwargs, init_timestamp, provider._session) | ||
| else: | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
| return loop.run_until_complete( | ||
| provider.handle_response(response, kwargs, init_timestamp, provider._session) | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Error in Fireworks completion: {str(e)}") | ||
| raise | ||
|
|
||
| self.client.chat.completions.create = patched_function | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Avoid Creating New Event Loops in Asynchronous Code
The current implementation creates a new event loop for each call in the _override_fireworks_completion method. This approach can lead to resource leaks and unexpected behavior, especially if the application already has an existing event loop. Instead, you should use the existing event loop by calling asyncio.get_event_loop(). This change will ensure that the event loop is managed correctly and efficiently, preventing potential resource exhaustion and maintaining the application's stability.
🔧 Suggested Code Diff:
def _override_fireworks_completion(self):
"""Override synchronous completion method."""
original_create = self._original_completion
provider = self
def patched_function(*args, **kwargs):
try:
init_timestamp = time.time()
response = original_create(*args, **kwargs)
if kwargs.get("stream", False):
return provider.handle_response(response, kwargs, init_timestamp, provider._session)
else:
- loop = asyncio.new_event_loop()
- asyncio.set_event_loop(loop)
+ loop = asyncio.get_event_loop()
return loop.run_until_complete(
provider.handle_response(response, kwargs, init_timestamp, provider._session)
)
except Exception as e:
logger.error(f"Error in Fireworks completion: {str(e)}")
raise
self.client.chat.completions.create = patched_function📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def _override_fireworks_completion(self): | |
| """Override synchronous completion method.""" | |
| original_create = self._original_completion | |
| provider = self | |
| def patched_function(*args, **kwargs): | |
| try: | |
| init_timestamp = time.time() | |
| response = original_create(*args, **kwargs) | |
| if kwargs.get("stream", False): | |
| return provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| else: | |
| loop = asyncio.new_event_loop() | |
| asyncio.set_event_loop(loop) | |
| return loop.run_until_complete( | |
| provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| ) | |
| except Exception as e: | |
| logger.error(f"Error in Fireworks completion: {str(e)}") | |
| raise | |
| self.client.chat.completions.create = patched_function | |
| def _override_fireworks_completion(self): | |
| """Override synchronous completion method.""" | |
| original_create = self._original_completion | |
| provider = self | |
| def patched_function(*args, **kwargs): | |
| try: | |
| init_timestamp = time.time() | |
| response = original_create(*args, **kwargs) | |
| if kwargs.get("stream", False): | |
| return provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| else: | |
| loop = asyncio.get_event_loop() | |
| return loop.run_until_complete( | |
| provider.handle_response(response, kwargs, init_timestamp, provider._session) | |
| ) | |
| except Exception as e: | |
| logger.error(f"Error in Fireworks completion: {str(e)}") | |
| raise | |
| self.client.chat.completions.create = patched_function |
📜 Guidelines
• Use exception handling for error management, but avoid overuse of try/except
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major modification that can potentially break existing code that relies on the previous return type. This change should be carefully reviewed and tested to ensure compatibility with all consumers of this function.
Actionable Steps:
- Review Usage: Thoroughly review all instances where this function is called to ensure they can handle a dictionary return type.
- Update Documentation: Ensure that all relevant documentation and type hints are updated to reflect this change.
- Migration Guide: If this function is part of a public API, consider providing a migration guide to assist users in adapting to the new return type.
- Testing: Implement comprehensive tests to verify that the new return type is correctly handled across the codebase.
This change is critical and should be approached with caution to avoid introducing bugs or breaking existing functionality. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
Co-Authored-By: Alex Reibman <[email protected]>
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a major modification that can have widespread implications on the codebase. This alteration can lead to runtime errors or unexpected behavior if the calling code is not updated to handle the new return type.
Actionable Steps:
- Review Usage: Thoroughly review all instances where this function is called to ensure they are compatible with the new return type.
- Update Dependent Code: Modify any code that processes the return value to handle a dictionary instead of a decimal.
- Migration Guide: If this function is part of a public API, consider providing a migration guide or a deprecation warning to inform users of the change.
- Testing: Implement comprehensive tests to verify that the new return type is handled correctly across the application.
This change is critical and should be approached with caution to prevent breaking existing functionality. 🛠️
| is_auto_end (bool, optional): is this an automatic use of end_session and should be skipped with skip_auto_end_session | ||
|
|
||
| Returns: | ||
| Decimal: The token cost of the session. Returns 0 if the cost is unknown. | ||
| Dict[str, Any]: Session statistics including duration, cost, and event counts. | ||
| """ | ||
| session = self._safe_get_session() | ||
| if session is None: | ||
| return | ||
| return None | ||
| if is_auto_end and self._config.skip_auto_end_session: | ||
| return | ||
|
|
||
| token_cost = session.end_session(end_state=end_state, end_state_reason=end_state_reason, video=video) | ||
| return None | ||
|
|
||
| return token_cost | ||
| return session.end_session(end_state=end_state, end_state_reason=end_state_reason, video=video) | ||
|
|
||
| def create_agent( | ||
| self, |
There was a problem hiding this comment.
🤖 Bug Fix:
Ensure Compatibility with Updated Return Type
The recent change in the return type from Decimal to Dict[str, Any] for session statistics is a significant modification that can lead to logical errors if not handled properly. This change requires a thorough review of all calling code to ensure compatibility with the new return type.
Actionable Steps:
- Audit Calling Code: Identify all instances where this function is called and ensure that the calling code is updated to handle a dictionary instead of a decimal.
- Update Logic: Modify any logic that processes the return value to extract and use the necessary information from the dictionary, such as cost, duration, and event counts.
- Testing: Implement comprehensive tests to verify that the updated logic correctly handles the new return type and that no runtime errors occur.
This change is critical as it affects the core functionality of session handling and could lead to incorrect behavior if not addressed properly. 🛠️
📜 Guidelines
• Encourage type annotations for clarity and error reduction
| def generator(stream_response): | ||
| accumulated_content = "" | ||
| try: | ||
| for chunk in stream_response: | ||
| if hasattr(chunk, "choices") and chunk.choices: | ||
| content = ( | ||
| chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, "content") else "" | ||
| ) | ||
| if content: | ||
| accumulated_content += content | ||
| yield chunk | ||
| # Update event with final accumulated content | ||
| event.completion = accumulated_content | ||
| event.end_timestamp = time.time() | ||
| if current_session: | ||
| current_session.record(event) | ||
| logger.info("Recorded streaming LLM event") | ||
| except Exception as e: | ||
| logger.error(f"Error in generator: {str(e)}") | ||
| raise |
There was a problem hiding this comment.
🤖 Bug Fix:
Ensure Safe Access to 'event' and 'current_session' in Generator
The current implementation of the generator function does not check if event and current_session are defined before accessing their properties or methods. This can lead to runtime errors if these objects are not properly initialized. To prevent such issues, it's crucial to add checks to ensure these objects are not None before using them. Additionally, consider implementing error handling or default values to maintain functionality even if these objects are not initialized.
Actionable Steps:
- Add checks to verify that
eventandcurrent_sessionare notNonebefore accessing their properties or methods. - Implement error handling to manage cases where these objects might be uninitialized, possibly by logging a warning or using default values.
🔧 Suggested Code Diff:
def generator(stream_response):
accumulated_content = ""
try:
for chunk in stream_response:
if hasattr(chunk, "choices") and chunk.choices:
content = (
chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, "content") else ""
)
if content:
accumulated_content += content
yield chunk
# Update event with final accumulated content
+ if event is not None:
event.completion = accumulated_content
event.end_timestamp = time.time()
+ else:
+ logger.warning("Event object is None, cannot update completion.")
if current_session is not None:
current_session.record(event)
logger.info("Recorded streaming LLM event")
+ else:
+ logger.warning("Current session is None, cannot record event.")
except Exception as e:
logger.error(f"Error in generator: {str(e)}")
raise📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def generator(stream_response): | |
| accumulated_content = "" | |
| try: | |
| for chunk in stream_response: | |
| if hasattr(chunk, "choices") and chunk.choices: | |
| content = ( | |
| chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, "content") else "" | |
| ) | |
| if content: | |
| accumulated_content += content | |
| yield chunk | |
| # Update event with final accumulated content | |
| event.completion = accumulated_content | |
| event.end_timestamp = time.time() | |
| if current_session: | |
| current_session.record(event) | |
| logger.info("Recorded streaming LLM event") | |
| except Exception as e: | |
| logger.error(f"Error in generator: {str(e)}") | |
| raise | |
| def generator(stream_response): | |
| accumulated_content = "" | |
| try: | |
| for chunk in stream_response: | |
| if hasattr(chunk, "choices") and chunk.choices: | |
| content = ( | |
| chunk.choices[0].delta.content if hasattr(chunk.choices[0].delta, "content") else "" | |
| ) | |
| if content: | |
| accumulated_content += content | |
| yield chunk | |
| # Update event with final accumulated content | |
| if event is not None: | |
| event.completion = accumulated_content | |
| event.end_timestamp = time.time() | |
| else: | |
| logger.warning("Event object is None, cannot update completion.") | |
| if current_session is not None: | |
| current_session.record(event) | |
| logger.info("Recorded streaming LLM event") | |
| else: | |
| logger.warning("Current session is None, cannot record event.") | |
| except Exception as e: | |
| logger.error(f"Error in generator: {str(e)}") | |
| raise |
📜 Guidelines
• Use pylint to catch bugs and style issues
• Use f-strings or format methods for string formatting
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major modification that can lead to runtime errors if the calling code is not updated. This change could break existing functionality that expects a Decimal return type.
Actionable Steps:
- Review Usage: Thoroughly review all instances where this function is called to ensure compatibility with the new return type.
- Update Dependent Code: Modify any dependent code to handle a
Dict[str, Any]return type instead ofDecimal. - Migration Guide: Consider providing a migration guide or deprecation warning if this change is part of a larger refactor. This will help users transition smoothly.
- Testing: Implement comprehensive tests to ensure that the new return type does not introduce bugs or unexpected behavior.
This change is critical and should be handled with care to avoid breaking existing systems. 🛠️
📜 Guidelines
• Encourage type annotations for clarity and error reduction
Co-Authored-By: Alex Reibman <[email protected]>
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Ensure Compatibility with New Return Type
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is significant and can lead to logical errors if not handled properly. Here are the steps to address this:
- Review Usage: Thoroughly review all instances where this function is called to ensure that the new return type is handled correctly. This includes checking for any operations that expect a
Decimaland updating them to handle a dictionary. - Update Documentation: Modify the function's docstring to clearly describe the new return type and the structure of the dictionary being returned. This will help other developers understand the expected output.
- Test Cases: Update or add test cases to cover scenarios with the new return type. This ensures that the function behaves as expected and helps catch any issues early.
- Communicate Changes: Inform the team about this change, especially if this function is part of a public API or widely used module, to prevent any unexpected runtime errors.
By following these steps, you can mitigate the risk of introducing bugs due to this change. 🛠️
📜 Guidelines
• Use docstrings for modules, functions, and classes • Use type annotations to improve code clarity and maintainability • Write unit tests for your code
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Return Type Change Detected
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major modification that can have widespread implications on the codebase. This alteration could lead to unexpected behavior in any code that relies on the previous return type, potentially causing runtime errors or logical bugs if not handled properly.
Actionable Steps:
- Review the Function Implementation: Ensure that the function's logic aligns with the new return type. Verify that the function indeed returns a
Dict[str, Any]in all scenarios where it previously returned aDecimal. - Update Dependent Code: Identify all instances where this function is called and update them to handle a dictionary return type. This may involve modifying how the return value is processed or stored.
- Backward Compatibility: Consider if backward compatibility is necessary. If so, provide a transitional solution or overload the function to support both return types temporarily.
- Testing: Implement comprehensive tests to cover the new return type. Ensure that all edge cases are considered and that the function behaves as expected in all scenarios.
By following these steps, you can mitigate the risks associated with this change and ensure that the codebase remains stable and functional. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
Co-Authored-By: Alex Reibman <[email protected]>
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant API Change: Return Type Modification
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a significant alteration in the function's API. This modification can lead to runtime errors in any part of the codebase that relies on the previous return type. It is crucial to:
- Review all usages of this function across the codebase to ensure compatibility with the new return type.
- Update dependent code to handle the new return type appropriately. This might involve changes in how the return value is processed or stored.
- Consider providing a migration guide if this function is part of a public API, to assist users in adapting to the change.
This change should be communicated clearly to all stakeholders to prevent unexpected issues in production environments. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| is_auto_end (bool, optional): is this an automatic use of end_session and should be skipped with skip_auto_end_session | ||
|
|
||
| Returns: | ||
| Decimal: The token cost of the session. Returns 0 if the cost is unknown. | ||
| Dict[str, Any]: Session statistics including duration, cost, and event counts. | ||
| """ | ||
| session = self._safe_get_session() | ||
| if session is None: | ||
| return | ||
| return None | ||
| if is_auto_end and self._config.skip_auto_end_session: | ||
| return | ||
|
|
||
| token_cost = session.end_session(end_state=end_state, end_state_reason=end_state_reason, video=video) | ||
| return None | ||
|
|
||
| return token_cost | ||
| return session.end_session(end_state=end_state, end_state_reason=end_state_reason, video=video) | ||
|
|
||
| def create_agent( | ||
| self, |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Return Type from Decimal to Dictionary
The change in the return type from Decimal to Dict[str, Any] is a significant modification that can impact any existing code relying on the previous return type. This change requires a thorough review of all dependent code to ensure compatibility with the new return type.
Actionable Steps:
- Review Usage: Identify all instances where this function is called and ensure that the calling code is updated to handle a dictionary instead of a
Decimal. - Update Documentation: Ensure that all documentation and comments reflect the new return type and its structure.
- Testing: Implement comprehensive tests to verify that the function behaves as expected with the new return type and that all dependent code is functioning correctly.
- Communicate Changes: Inform all team members and stakeholders about this change to prevent any unexpected issues in the production environment.
This change is critical and should be handled with care to avoid breaking existing functionality. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major modification that can impact the functionality of the code. This change requires a thorough review of all parts of the codebase that interact with this function to ensure they handle the new return type correctly.
Actionable Steps:
- Review Calling Code: Examine all instances where this function is called to ensure they can handle a dictionary return type instead of a
Decimal. - Update Logic: Modify any logic that processes the return value to accommodate the new dictionary structure.
- Unit Tests: Update existing unit tests to reflect the new return type and add new tests if necessary to cover edge cases.
- Documentation: Update any relevant documentation to reflect the change in the return type.
This change is critical and should be handled with care to avoid introducing bugs into the system. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
Co-Authored-By: Alex Reibman <[email protected]>
Co-Authored-By: Alex Reibman <[email protected]>
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a major alteration that can impact any code relying on this function. This change requires a thorough review of all instances where this function is used to ensure compatibility with the new return type.
Actionable Steps:
- Audit Usage: Review all codebases where this function is called to identify potential issues with the new return type.
- Update Dependent Code: Modify any dependent code to handle the
Dict[str, Any]return type appropriately. This may involve changes in how the return value is processed or stored. - Documentation: Update the function's documentation to reflect the new return type and provide examples of how to handle it.
- Migration Guide: Consider creating a migration guide to assist developers in adapting to this change, especially if this function is widely used.
This change is critical and should be communicated clearly to all developers involved in the project to prevent runtime errors or logical issues. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major alteration in the function's API. This can lead to runtime errors if the calling code expects a Decimal and now receives a Dict.
Actionable Steps:
- Review Usage: Thoroughly review all instances where this function is called to ensure they handle the new return type correctly.
- Update Documentation: Modify the function's documentation to reflect the new return type and provide guidance on how to handle the
Dictreturn value. - Testing: Implement comprehensive tests to verify that the function behaves as expected with the new return type and that all dependent code is updated accordingly.
This change is critical and should be handled with care to avoid breaking existing functionality. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Method Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major modification that can have widespread implications on the codebase. This alteration requires a thorough review of all the places where this method is invoked to ensure compatibility with the new return type.
Actionable Steps:
- Review Usage: Identify all instances where this method is called and verify that the logic handling the return value is updated to process a dictionary instead of a decimal.
- Update Documentation: Ensure that the method's documentation is updated to reflect the new return type and its structure.
- Communicate Changes: Inform all relevant stakeholders about this change, especially those who maintain code that interacts with this method.
- Testing: Implement comprehensive tests to validate that the new return type is correctly handled across the application.
This change is critical and should be approached with caution to prevent any runtime errors or logical inconsistencies in the application.
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
Co-Authored-By: Alex Reibman <[email protected]>
…r tests Co-Authored-By: Alex Reibman <[email protected]>
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a major modification that can have widespread implications on the codebase. This change can lead to runtime errors or incorrect behavior if the calling code is not updated to handle the new return type.
Actionable Steps:
- Review Usage: Thoroughly review all instances where this function is called to ensure they are compatible with the new return type.
- Update Dependent Code: Modify any code that processes the return value to handle a dictionary instead of a decimal.
- Documentation: Update the function's documentation to clearly describe the new return type and its structure.
- Migration Path: Consider providing a migration guide or notes to assist other developers in adapting their code to this change.
This change should be communicated clearly to the team to prevent any unexpected issues in the application. 📢
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major modification that can have widespread implications on the codebase. This alteration requires a thorough review of all instances where this function is called to ensure compatibility with the new return type.
Actionable Steps:
- Audit Usage: Identify all places in the code where this function is used and verify that they can handle a dictionary return type.
- Update Dependent Code: Modify any dependent code that expects a
Decimalto correctly process aDict[str, Any]. - Type Checks: Consider adding type checks or assertions within the function to ensure the return value is consistent with the new type.
- Documentation: Update the function's documentation to reflect the new return type and provide examples of how to handle it.
This change is critical and should be handled with care to prevent runtime errors and ensure the stability of the application. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| is_auto_end: Optional[bool] = None, | ||
| ) -> Optional[Decimal]: | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| End the current session with the AgentOps service. | ||
|
|
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Optional[Decimal] to Optional[Dict[str, Any]] is a major modification that can potentially break existing code that depends on the previous return type. This change requires a thorough review of all code that calls this function to ensure compatibility with the new return type.
Actionable Steps:
- Review Usage: Identify all instances where this function is used and verify that the calling code can handle a
Dict[str, Any]instead of aDecimal. - Update Dependent Code: Modify any dependent code to correctly process the new return type. This might involve changes in how the return value is accessed and used.
- Documentation: Update any relevant documentation to reflect this change. If this function is part of a public API, consider providing a migration guide for external users.
- Testing: Ensure that unit tests are updated or added to cover the new return type and its usage scenarios.
This change is critical and should be handled with care to avoid introducing bugs into the system. 🛠️
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
| end_state: str = "Indeterminate", | ||
| end_state_reason: Optional[str] = None, | ||
| video: Optional[str] = None, | ||
| ) -> Union[Decimal, None]: | ||
| ) -> Union[Dict[str, Any], None]: | ||
| with self._end_session_lock: | ||
| if not self.is_running: | ||
| return None |
There was a problem hiding this comment.
🤖 Bug Fix:
Significant Change in Function Return Type
The change in the return type from Union[Decimal, None] to Union[Dict[str, Any], None] is a major modification that can have widespread implications across the codebase. This alteration requires a thorough review of all instances where this function is called to ensure compatibility with the new return type.
Actionable Steps:
- Review Usage: Identify all places in the code where this function is used and verify that they can handle a
Dict[str, Any]return type instead of aDecimal. - Update Dependent Code: Modify any dependent code to correctly process the dictionary return type. This may involve changes in how the return value is accessed and used.
- Testing: Implement tests to validate the new behavior of the function. Ensure that these tests cover edge cases and verify that the function behaves as expected with the new return type.
- Backward Compatibility: Consider if maintaining backward compatibility is necessary. If so, provide a transitional solution or document the change clearly for users of the function.
This change is critical and should be handled with care to prevent runtime errors or logical issues in the application.
📜 Guidelines
• Use type annotations to improve code clarity and maintainability
• Write unit tests for your code
🔍 Review Summary
Purpose
This update aims to integrate a new service provider, Fireworks, into the system. By enabling synchronous and asynchronous completions and providing streaming capabilities, the update allows more seamless interaction with Fireworks AI's Language Model (LLM) models.
Changes
New Feature
FireworksProviderclass compatible with OpenAI's interface.Documentation
Enhancement
Refactor
agentops/llms/tracker.py.Impact
The update enriches user experience with practical examples and comprehensive documentation, facilitating the integration of a new service provider. This opens up opportunities for more robust and diverse applications.
Original Description
No existing description found