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.

Sign up for GitHub

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

Jump to bottom

[ENH] Handle partial nodes success #55

Merged
merged 21 commits into from
Jan 14, 2024
Merged

[ENH] Handle partial nodes success #55

merged 21 commits into from
Jan 14, 2024

Conversation

alyssadai
Copy link
Contributor

@alyssadai alyssadai commented Jan 11, 2024 •
edited by surchs
Loading

Changes proposed in this pull request:

  • New response model for federated response that includes node-specific errors and successful query results, as well as an overall nodes response status
  • When >=1 nodes fail, use a custom 207 HTTP success code for overall response from API
  • Print any encountered node errors to the console, as well as summary statement about # nodes successfully queried
  • When querying individual nodes, gracefully handle any network/node connection errors related to the request (i.e., if node API is unreachable)
  • Switch to async process for sending out individual node requests

Checklist

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see https://neurobagel.org/contributing/pull_requests for more info)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

@alyssadai
add comment explaining FEDERATION_NODES format
8de7904
@alyssadai
implement custom HTTP response for partial success federated query
780b615 
- use custom HTTP success status code
- return both node-specific errors and combined successful query results in response body
- log node errors and query federation successfulness to console
@alyssadai
test API path response for partial success federated query
3643130
@alyssadai
mock individual node request to assert over combined query response
566e7e1
@alyssadai
add new response model returning errors and node query results
5502a49
@alyssadai
do not use exception object for partial success responses
16114da
@alyssadai
rename status key in combined query response
fe9703f
@alyssadai
update response returned when all nodes succeed or fail
9fa13ea
@alyssadai
handle network errors in federated query
05be17f
@alyssadai
test federated response given unreachable nodes, create fixture for s…
1ea377a 
…ingle matching dataset result
@alyssadai
refactor tests of federated query response into separate module
e338599
@alyssadai
test response when queries to all nodes fail
caa3933
@alyssadai
test response when queries to all nodes succeed
d0f858b
@alyssadai
update comments and type hints
4b9e926
@alyssadai
turn status of node responses into enum
e1eafdc
@alyssadai
use model for node error in federated query response
b7fbb60
@alyssadai
switch to sending requests to nodes asynchronously
cf8d392
@alyssadai
update mocked get function in tests to be async
83a50cd
@coveralls
Copy link
Collaborator

coveralls commented Jan 11, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 7521932835

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.6%) to 99.314%
Totals Coverage Status
Change from base Build 7266039620: 0.6%
Covered Lines: 579
Relevant Lines: 583
💛 - Coveralls

@surchs surchs self-requested a review January 11, 2024 16:40
surchs
surchs approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Thank you very much @alyssadai for this great PR 🎉 ! I left some comments but otherwise 🧑‍🍳

app/api/crud.py Show resolved Hide resolved
app/api/crud.py Show resolved Hide resolved
app/api/models.py Show resolved Hide resolved
app/api/crud.py Outdated Show resolved Hide resolved
app/api/crud.py Show resolved Hide resolved
app/api/crud.py Show resolved Hide resolved
tests/test_query.py Outdated Show resolved Hide resolved
tests/test_query.py Show resolved Hide resolved
alyssadai and others added 2 commits January 14, 2024 16:34
@alyssadai @surchs
make code a bit cleaner
f65aff8 
Co-authored-by: Sebastian Urchs <surchs@users.noreply.github.com>
@alyssadai
rename fixture for mocked data
f3640e6
@alyssadai alyssadai merged commit 3e76fd0 into main Jan 14, 2024
5 checks passed
@alyssadai alyssadai deleted the handle-partial-nodes-success branch January 14, 2024 22:59
@alyssadai alyssadai mentioned this pull request Feb 21, 2024
Closed
17 tasks
@surchs
Copy link
Contributor

surchs commented Apr 11, 2024

🚀 PR was released in v0.1.0 🚀

@surchs surchs added the released This issue/pull request has been released. label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surchs surchs surchs approved these changes

Assignees
No one assigned
Labels
released This issue/pull request has been released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle cases where requests to some nodes fail
3 participants
@alyssadai @coveralls @surchs