Skip to content

[ENH] Handle partial nodes success#55

Merged
alyssadai merged 21 commits intomainfrom
handle-partial-nodes-success
Jan 14, 2024
Merged

[ENH] Handle partial nodes success#55
alyssadai merged 21 commits intomainfrom
handle-partial-nodes-success

Conversation

@alyssadai
Copy link
Copy Markdown
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 added 18 commits January 8, 2024 22:33
@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
Copy Markdown
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
Copy Markdown
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 🧑‍🍳

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
@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
Copy Markdown
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