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

Add num dpus parameter to query offline API #638

Merged
merged 5 commits into from
Feb 13, 2025

Conversation

saiharshavellanki
Copy link
Contributor

@saiharshavellanki saiharshavellanki commented Feb 13, 2025

Add num_dpus parameter to query_offline() in client.py and mock_client.py to specify Glue DPUs for offline queries.

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.

❌ Changes requested. Reviewed everything up to 958a2db in 1 minute and 28 seconds

More details
  • Looked at 43 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. fennel/client/client.py:461
  • Draft comment:
    New parameter num_dpus added for query_offline. Consider validating that num_dpus (if provided) is a positive integer to prevent unintended values.
  • Reason this comment was not posted:
    Marked as duplicate.
2. fennel/testing/mock_client.py:310
  • Draft comment:
    New num_dpus parameter added to query_offline signature in MockClient; currently it isn’t used. Consider documenting that DPUs are ignored in the mock implementation.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_TIRxCWA7Y1dZir6g


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

Copy link

@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 607de22 in 46 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. fennel/client/client.py:461
  • Draft comment:
    Consider adding a validation to ensure that if provided, 'num_dpus' is a positive integer.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. fennel/client/client.py:462
  • Draft comment:
    Trailing comma in the parameter list improves multi-line formatting. Looks good!
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_mZEgbNWwIUggTDvc


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 6d6b165 in 39 seconds

More details
  • Looked at 24 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. fennel/client/client.py:632
  • Draft comment:
    Consider validating that 'num_dpus' is a positive integer before adding it to the request payload. This would prevent invalid values and clarify its expected type in the docstring.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. fennel/client/client.py:632
  • Draft comment:
    Consider adding a validation to ensure that 'num_dpus', if provided, is a positive integer.
  • 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_8fXAP1WdCbtWt7Eo


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

Copy link
Contributor

@nonibansal nonibansal left a comment

Choose a reason for hiding this comment

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

Upgrade version in pyproject.toml file and update documentation.

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 892f127 in 1 minute and 22 seconds

More details
  • Looked at 23 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. fennel/CHANGELOG.md:2
  • Draft comment:
    The changelog version order seems off: [1.5.77] has a date earlier than [1.5.76]. Confirm that the version and dates are correct.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
    The comment identifies a real inconsistency - version 1.5.77 has an earlier date than 1.5.76, which doesn't make sense chronologically. This could indicate a real issue with the versioning or dates. However, the comment asks for confirmation ("Confirm that...") which violates our rules about not asking for confirmations. Additionally, changelogs sometimes intentionally have non-chronological entries when backporting fixes.
    I may be being too strict about the "no confirmations" rule - this could be a legitimate versioning error that needs fixing. The suggested fix of 2025-02-14 seems reasonable.
    While the issue identified is real, asking for confirmation violates our core rules. If there's a clear versioning error, it should be stated directly rather than asking for verification.
    Delete this comment. While it identifies a real inconsistency, it asks for confirmation rather than directly stating the issue. Additionally, non-chronological changelog entries can be valid in some cases like backports.
2. pyproject.toml:3
  • Draft comment:
    Version updated to 1.5.77. Ensure this bump is consistent with other docs.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. fennel/CHANGELOG.md:2
  • Draft comment:
    Release date ordering seems off: version 1.5.77 is dated 2024-11-12, but the following 1.5.76 has 2025-02-13. Please verify correct order.
  • Reason this comment was not posted:
    Marked as duplicate.
4. pyproject.toml:3
  • Draft comment:
    Version bump to 1.5.77 here is consistent with the changelog. Confirm all metadata references are updated.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_GfoXVtWdIJpVrDzg


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.

❌ Changes requested. Incremental review on 37b7337 in 1 minute and 36 seconds

More details
  • Looked at 23 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. pyproject.toml:3
  • Draft comment:
    Version bump updated to 1.5.78. Ensure this change aligns with changelog and release notes.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. fennel/CHANGELOG.md:2
  • Draft comment:
    Check version/date order: 1.5.78 (2024-11-12) is listed above 1.5.77 (2025-02-13). Confirm intended chronology.
  • Reason this comment was not posted:
    Marked as duplicate.
3. pyproject.toml:3
  • Draft comment:
    Version bump to 1.5.78 looks correct and is in sync with the changelog.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_uxkujOUid9qduIPV


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

@saiharshavellanki saiharshavellanki merged commit be0de19 into main Feb 13, 2025
8 checks passed
@saiharshavellanki saiharshavellanki deleted the query_offline_api_update branch February 13, 2025 05:09
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