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

[ENH] Make data upload script backend responsive #205

Merged
merged 6 commits into from
Oct 22, 2023

Conversation

alyssadai
Copy link
Contributor

@alyssadai alyssadai commented Oct 19, 2023

Closes #204

Changes proposed in this pull request:

  • Add bool flag for whether to use GraphDB API endpoints (if off, uses Stardog endpoints)
  • For each request to the graph (clearing data or uploading data), explicitly capture response codes and increase verbosity for easier debugging
    • If database clearing step fails (when selected), exit script instead of continuing to upload the data files
    • At end of process, output to user a list of any db filenames that didn't succeed in uploading (status code != 2xx)
  • Capitalize script output messages for easier visual differentiation from HTTP error messages

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.

@coveralls
Copy link
Collaborator

coveralls commented Oct 19, 2023

Pull Request Test Coverage Report for Build 6601490156

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.826%

Totals Coverage Status
Change from base Build 6565267296: 0.0%
Covered Lines: 575
Relevant Lines: 576

💛 - Coveralls

@alyssadai alyssadai changed the title [ENH] Adai/make data upload script backend responsive [ENH] Make data upload script backend responsive Oct 19, 2023
@rmanaem rmanaem self-requested a review October 20, 2023 17:38
Copy link
Collaborator

@rmanaem rmanaem left a comment

Choose a reason for hiding this comment

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

The changes look good.
However, I do have a question and a suggestion.
I'm curious to know why you capitalized the messages? Is it a convention?
🍒 : I think it's a good idea to rename either graph-db or use-graphdb-syntax in code for the sake of clarity e.g., use-GraphDB-syntax.

add_data_to_graph.sh Show resolved Hide resolved
add_data_to_graph.sh Show resolved Hide resolved
rmanaem

This comment was marked as spam.

rmanaem

This comment was marked as spam.

@alyssadai
Copy link
Contributor Author

I'm curious to know why you capitalized the messages? Is it a convention?

Mostly so that it is easy for the user to visually distinguish the progress messages from the script itself (e.g. CLEARING EXISTING DATA ...) and the HTTP responses returned from the actual graph API which are also printed to the console (since all the curl commands for uploading data are called in a way as to output the response headers for debugging). Otherwise, the output from the script may be a bit tricky to parse.

🍒 : I think it's a good idea to rename either graph-db or use-graphdb-syntax in code for the sake of clarity e.g., use-GraphDB-syntax.

Good point! I've renamed the graph-db parameter to graph-database 🙂

@alyssadai alyssadai merged commit 0b44bcd into main Oct 22, 2023
4 checks passed
@alyssadai alyssadai deleted the adai/make-data-upload-script-backend-responsive branch October 22, 2023 03:51
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.

Add flag to add_data_to_graph.sh for graph backend and increase verbosity
3 participants