Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add base url for OpenAI connection #166

Merged
merged 4 commits into from
Feb 27, 2025
Merged

Conversation

TrachukT
Copy link
Contributor

@TrachukT TrachukT commented Feb 26, 2025

Important

Add configurable base URL for OpenAI connection in connections.py and update tests in test_flow.py.

  • Behavior:
    • Add url attribute to OpenAI in connections.py, defaulting to https://api.openai.com/v1.
    • Modify connect() in OpenAI to use base_url parameter.
    • Add conn_params property to return connection parameters including api_base.
  • Tests:
    • Update test_flow.py to check api_base parameter in mock_llm_executor calls.
    • Ensure tests verify the correct URL is used in OpenAI connections.

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

@TrachukT TrachukT requested a review from a team as a code owner February 26, 2025 13:29
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 6173148 in 2 minutes and 23 seconds

More details
  • Looked at 65 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. dynamiq/connections/connections.py:192
  • Draft comment:
    Good addition of the 'url' field with a default. Consider updating the docstring to clearly state the default value and environment variable for clarity.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment suggests updating a docstring, which is purely informative and not a code suggestion or a request for a test. It doesn't align with the rules for good comments.
2. dynamiq/connections/connections.py:205
  • Draft comment:
    Ensure that the underlying OpenAIClient supports the 'base_url' parameter. Document this parameter if it’s a custom extension.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking to ensure that a specific parameter is supported and to document it if it's a custom extension. This is not a specific code suggestion or a request for a test, but rather a general request to ensure something is supported and documented. It seems to violate the rule against asking the PR author to ensure something is intended or tested.
3. dynamiq/connections/connections.py:211
  • Draft comment:
    Minor: The docstring for conn_params could be improved for readability (e.g., add spaces after dashes).
  • 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%
    While the suggestion would technically improve readability slightly, this is an extremely minor formatting issue that doesn't affect functionality. The docstring is still perfectly readable and understandable in its current form. Comments about pure formatting/style issues should generally be avoided unless they significantly impact code quality.
    The formatting inconsistency could propagate if other developers copy this style. Having consistent docstring formatting across the codebase is valuable.
    While consistency is good, this level of nitpicking about spaces in docstrings creates noise in the PR review process and distracts from more important issues. The benefit is too small to justify the comment.
    Delete this comment as it's an extremely minor formatting suggestion that doesn't meaningfully impact code quality or readability.
4. tests/integration/flows/test_flow.py:126
  • Draft comment:
    The api_base parameter "https://api.openai.com/v1" is now explicitly passed in the test. Confirm that this is consistently required by the OpenAI connection.
  • 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 if the explicit passing of the api_base parameter is consistently required. This falls under asking the author to confirm their intention, which is against the rules.
5. dynamiq/connections/connections.py:193
  • Draft comment:
    The OpenAI connection field for the URL is defined as 'url', but the conn_params property returns it under the key 'api_base'. Consider using consistent naming (e.g., use 'api_base' everywhere) to avoid confusion.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
    The inconsistency could potentially cause confusion for developers. However, looking at the code, both the field name 'url' and parameter name 'api_base' are fairly clear in their meaning. The OpenAI client accepts 'base_url' as the parameter name, so neither naming choice perfectly matches the client anyway. This seems more like a minor stylistic preference than a significant issue.
    The comment might be overstating the importance of this naming consistency issue. The code works correctly and both names are reasonably clear.
    While consistency is good, in this case both names are self-explanatory and the code functions correctly. The benefit of changing this would be minimal.
    This comment points out a real but minor inconsistency. The current names are clear enough that changing them would provide little value.
6. dynamiq/connections/connections.py:207
  • Draft comment:
    Verify that the OpenAIClient initializer accepts the keyword 'base_url'. The connect() method uses 'base_url' while tests and conn_params refer to 'api_base'. Consistency here will reduce potential bugs.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
7. tests/integration/flows/test_flow.py:123
  • Draft comment:
    The test now includes an explicit 'api_base' parameter in the mock call to openai. Consider deriving this expected value from the connection's configuration to minimize hardcoding and ensure consistency if defaults change.
  • 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%
    This is a test file that verifies mock call arguments. The purpose is to assert that the code under test makes the right API calls with the right parameters. Hardcoding expected values in tests is normal and often desirable for readability and explicit verification. Deriving values makes tests more complex and can hide bugs if the derived value is wrong. The suggestion would make the tests more complex without clear benefit.
    Maybe hardcoding could cause test maintenance issues if the default API base URL changes in the future? Maybe there's a DRY principle being violated?
    Tests should be explicit and easy to understand. If the API base URL changes, the tests SHOULD fail to alert us that behavior changed. That's the point of tests.
    Delete the comment. Hardcoding expected values in tests is good practice for clarity and explicit verification. The suggestion would make tests more complex without benefit.
8. tests/integration/flows/test_flow.py:277
  • Draft comment:
    Similarly, the failure test now passes an explicit 'api_base' parameter. To avoid duplication of hardcoded strings, consider reusing the connection's default or a centralized setting.
  • 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.
9. dynamiq/connections/connections.py:190
  • Draft comment:
    Minor suggestion: In the OpenAI class docstring and attributes, consider capitalizing 'URL' instead of 'url' (e.g., 'endpoint URL' instead of 'endpoint url', and 'base URL' instead of 'base url') for consistency. Also, in the bullet list within the conn_params docstring, adding a space after the hyphen (e.g., '- the API key...') would improve readability.
  • 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%
    While the comment points out real inconsistencies, these are very minor style issues that don't affect functionality. The URL capitalization is a matter of preference, and the bullet point spacing is a trivial formatting detail. Neither issue requires immediate action or significantly impacts code quality or readability.
    The comment does identify actual inconsistencies in documentation style. Some might argue that consistent documentation formatting is important for professionalism.
    While consistency is good, these changes are too minor to warrant a PR comment. They could be handled in a broader documentation cleanup if needed.
    Delete this comment as it focuses on trivial formatting issues that don't meaningfully impact code quality or understanding.

Workflow ID: wflow_w9tsrbS9zPfsoXSU


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

Copy link

github-actions bot commented Feb 26, 2025

Coverage

Coverage Report •
FileStmtsMissCoverMissing
dynamiq/connections
   connections.py4265487%14–18, 58, 66, 77, 100, 477–478, 480–481, 485–486, 488–489, 502–503, 505, 531, 533, 545, 547–549, 623–624, 665, 765, 818, 863–864, 875, 969–970, 1003–1004, 1040–1041, 1090–1091, 1093, 1096–1097, 1102, 1107, 1114–1115, 1118–1122
TOTAL11912363069% 

Tests Skipped Failures Errors Time
416 0 💤 0 ❌ 0 🔥 45.507s ⏱️

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 1515f5b in 1 minute and 26 seconds

More details
  • Looked at 18 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. dynamiq/connections/connections.py:212
  • Draft comment:
    Docstring now omits return details. Consider including all keys (e.g., 'api_key') for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The comment is about documentation rather than functionality. While accurate that details were removed, docstring completeness is more of a style preference. The method is simple and the keys are self-evident from the code. The comment doesn't point out any actual issues that need fixing.
    The docstring could be helpful for API documentation generation. Missing documentation could make the code harder to maintain long-term.
    While documentation is good, this is a very simple method where the return value is clear from the code. The comment doesn't highlight a significant issue that needs addressing.
    Delete the comment. While technically accurate, it's more of a style suggestion than a necessary code change, and the method's behavior is clear without detailed docs.
2. dynamiq/connections/connections.py:213
  • Draft comment:
    The removal of detailed return information in the docstring may reduce clarity. Consider briefly noting the returned keys (e.g., 'api_base' and 'api_key') to maintain consistency with similar connection classes.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_nqQA7fmSXXkslOIt


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

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 2cb9b9f in 1 minute and 54 seconds

More details
  • Looked at 329 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 25 drafted comments based on config settings.
1. dynamiq/nodes/agents/base.py:52
  • Draft comment:
    Switching context from _prompt_variables to _prompt_blocks for rendering could affect prompt consistency. Ensure all templates expect 'context' in blocks.
  • 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 change appears intentional and correct. _prompt_blocks stores the template blocks that make up the final prompt structure, while _prompt_variables stores the values used to format those blocks. Looking at generate_prompt() method, it formats each block using the variables. The change actually makes more sense architecturally since the role is a template that needs to be rendered first.
    I could be missing some subtle interaction between the prompt generation system components. There could be code elsewhere that expects context to be in _prompt_variables.
    The code is quite clear - _prompt_blocks and _prompt_variables have distinct purposes, and the change aligns with those purposes. The generate_prompt() method shows exactly how they work together.
    The comment should be deleted. The change is architecturally sound and improves the separation of concerns between template blocks and variables.
2. dynamiq/nodes/agents/base.py:146
  • Draft comment:
    Field 'role' now defaults to an empty string. Confirm that downstream logic correctly handles an empty role.
  • 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. dynamiq/nodes/agents/react.py:226
  • Draft comment:
    Default inference_mode changed to DEFAULT instead of XML. Verify that this change aligns with intended agent behavior in production.
  • 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 if the change aligns with the intended behavior, which violates the rule against asking for confirmation of intention. It does not provide a specific suggestion or point out a clear issue with the code.
4. dynamiq/nodes/agents/react.py:297
  • Draft comment:
    Enhanced JSON parse error logging in parse_xml_and_extract_info improves debuggability. Confirm that error details do not expose sensitive information.
  • 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 comment appears to be speculative - it's asking to "confirm" something without evidence that there's actually a problem. The error logging shown is quite standard and contains only technical formatting details. The examples are intentionally generic. There's no indication that sensitive data could be exposed.
    I could be missing some context about what kind of sensitive data might be present in the action_input_text that could potentially be exposed in the error message.
    Even if action_input_text contained sensitive data, the error logging only shows the JSON parsing error message and generic examples, not the actual input content.
    This comment should be deleted as it raises a speculative concern without evidence of an actual issue, and the code shows that the error logging is appropriately generic.
5. dynamiq/nodes/tools/exa_search.py:24
  • Draft comment:
    New 'is_accessible_to_agent' flags added to schema fields enhance security by controlling agent access. Verify consistency across all tools.
  • 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 consistency across all tools, which is a form of asking them to double-check their work. This violates the rule against asking the author to confirm or ensure things. The comment does not provide a specific code suggestion or point out a specific issue.
6. dynamiq/nodes/tools/tavily.py:14
  • Draft comment:
    Consistent addition of is_accessible_to_agent flags to TavilyInputSchema fields; ensures tools expose only intended parameters to agents.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply acknowledges a change without offering any specific guidance or questions.
7. dynamiq/prompts/prompts.py:319
  • Draft comment:
    Removed duplicate implementation of parse_bytes_to_base64 and parse_image_url_parameters. This cleanup improves maintainability.
  • 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.
8. dynamiq/nodes/agents/base.py:52
  • Draft comment:
    Changed context block header from 'Additional context:' to 'Context:'. Ensure this formatting change aligns with existing templates.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
    The comment is asking the author to ensure that a formatting change aligns with existing templates. This is similar to asking the author to double-check or confirm their intention, which violates the rules. The comment does not provide a specific code suggestion or point out a potential issue with the code itself.
9. dynamiq/nodes/agents/base.py:120
  • Draft comment:
    Added an extra message with the role from context in AgentInputSchema. Confirm that the 'role' key is always provided in the input schema context.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50%
    The comment is asking the author to confirm that a 'role' key is always provided in the input schema context. This is a request for confirmation, which is not allowed by the rules. However, it does point out a potential issue with the 'role' key, which could be useful if rephrased to suggest checking or testing for the presence of the 'role' key.
10. dynamiq/nodes/agents/base.py:146
  • Draft comment:
    Changed the default for 'role' from Optional (None) to an empty string. Ensure any downstream logic properly handles an empty role.
  • 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.
11. dynamiq/nodes/agents/base.py:169
  • Draft comment:
    Updated get_context_for_input_schema() to include 'role'. Verify that validators expecting context handle the role appropriately.
  • 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. dynamiq/nodes/agents/base.py:222
  • Draft comment:
    Modified _init_prompt_blocks() by setting the 'context' block to an empty string instead of a template placeholder. Ensure this is intentional so that context data is only rendered via execute.
  • 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. dynamiq/nodes/agents/base.py:295
  • Draft comment:
    In execute(), the assignment of rendered context now goes to _prompt_blocks instead of _prompt_variables. Validate that template rendering will pick up the context correctly.
  • 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.
14. dynamiq/nodes/agents/react.py:226
  • Draft comment:
    Default inference_mode changed from XML to DEFAULT. Confirm that this new default is compatible with the intended response formatting and downstream processing.
  • 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 compatibility with the intended response formatting and downstream processing, which violates the rule against asking for confirmation of intention or ensuring behavior is intended. The comment does not provide a specific code suggestion or ask for a specific test to be written.
15. dynamiq/nodes/agents/react.py:297
  • Draft comment:
    Added a logger.error call upon JSON decode failure to aid in debugging. This improves traceability of JSON formatting issues.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, explaining what was done and why, without suggesting any changes or improvements. It doesn't align with the rules for useful comments.
16. dynamiq/nodes/tools/exa_search.py:25
  • Draft comment:
    Added 'is_accessible_to_agent' metadata to several fields. This ensures only intended parameters are exposed to agents. Verify consistency with other tools.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is informative and asks the author to verify consistency, which is against the rules. It doesn't provide a specific suggestion or point out a potential issue.
17. dynamiq/nodes/tools/tavily.py:14
  • Draft comment:
    Added 'is_accessible_to_agent=False' for multiple fields in TavilyInputSchema. Ensure these fields remain hidden from agent input when necessary.
  • 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 fields remain hidden from agent input when necessary. This is a request for confirmation of intended behavior, which violates the rules.
18. dynamiq/prompts/prompts.py:195
  • Draft comment:
    Adjusted Message formatting in VisionMessage.format_message by using keyword arguments for Message initialization. This improves clarity and 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.
19. dynamiq/prompts/prompts.py:320
  • Draft comment:
    Removed duplicate definitions of parse_bytes_to_base64 and parse_image_url_parameters. Verify that all references now call the intended implementation in VisionMessage.
  • 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.
20. dynamiq/nodes/agents/base.py:55
  • Draft comment:
    Minor typo: The template text on line 55 says 'Refer to this as to additional information, not as direct instructions.' Consider changing it to 'Refer to this as additional information, not as direct instructions.' for better clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.
21. dynamiq/nodes/agents/base.py:563
  • Draft comment:
    Typographical error: The variable 'formated_prompt_blocks' appears to be misspelled. It should likely be 'formatted_prompt_blocks' to maintain clarity and 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.
22. dynamiq/nodes/agents/react.py:84
  • Draft comment:
    Typographical Error: In the REACT_BLOCK_INSTRUCTIONS block, the word 'strickly' should be corrected to 'strictly' for clarity and 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.
23. dynamiq/nodes/tools/exa_search.py:31
  • Draft comment:
    Typo: The description for 'use_autoprompt' reads 'If true, query will be converted to a Exa query.' It should be 'an Exa query' to be grammatically correct.
  • Reason this comment was not posted:
    Comment was on unchanged code.
24. dynamiq/nodes/tools/exa_search.py:94
  • Draft comment:
    Typo: The description for 'use_autoprompt' in the ExaTool class reads 'If true, query will be converted to a Exa query.' It should be 'an Exa query' for correct grammar.
  • 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. dynamiq/prompts/prompts.py:73
  • Draft comment:
    Typographical error: In the Message.format_message docstring, "Returns formated copy of message" should be corrected to "Returns formatted copy of message".
  • 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_a1ANoityRARDNHdW


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

@acoola acoola merged commit e9b4e54 into main Feb 27, 2025
8 checks passed
@acoola acoola deleted the feat/update-openai-config branch February 27, 2025 12:56
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.

2 participants