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

fix: Fix response iterator in Python In-Process API #370

Closed
wants to merge 1 commit into from

Conversation

Tabrizian
Copy link
Member

@Tabrizian Tabrizian commented Jun 13, 2024

What does the PR do?

The response iterator destroyed the request object after final response is received which lead to destruction of the queue and responses not being retrieved by the user.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

N/A

Where should the reviewer start?

N/A

Test plan:

Updated L0_python_api.

  • CI Pipeline ID:
    15804851

Caveats:

N/A

Background

N/A

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

N/A

@Tabrizian Tabrizian force-pushed the imant-fix-response-queue branch from e959c98 to a80cd1c Compare June 13, 2024 16:27
@rmccorm4 rmccorm4 self-requested a review June 13, 2024 21:15
numpy.testing.assert_array_equal(input_value, output_value)
)

responses = list(response_iterator)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is subtle at a glance, but list(response_iterator) is actually blocking and iterating on the iterator until it's completely consumed, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. It looks like sometimes the previous version of the test didn't execute since there were no responses observed. This is to make sure that the response length is always one.

Comment on lines -186 to -187
del self._request
self._request = None
Copy link
Contributor

@rmccorm4 rmccorm4 Jun 14, 2024

Choose a reason for hiding this comment

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

From bindings / lifecycle perspectives, are there any cons introduced by not explicitly deleting the request here or somewhere else? Will an object get kept around unexpectedly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Memory usage appears to be stable. Might need stress tests to make sure it is not leaking any memory.

@GuanLuo
Copy link
Contributor

GuanLuo commented Jun 17, 2024

What is being held in request object? Aren't the responses being stored in the iterator?

@Tabrizian
Copy link
Member Author

@GuanLuo I think the response_iterator was relying on a queue inside the request object which might be the root cause for this issue.

@Tabrizian Tabrizian closed this Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants