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 predictor/collection upload route #611

Merged
merged 19 commits into from Jul 10, 2019
Merged

Add predictor/collection upload route #611

merged 19 commits into from Jul 10, 2019

Conversation

adelavega
Copy link
Collaborator

@adelavega adelavega commented Jul 3, 2019

  • Partially addresses Interface for uploading for custom annotations #603 by adding a route to upload custom predictors
  • Fixes an issue where the API Docs did not indicate file type correctly
  • To fix the above, and to better organize the core app, I separate the API docs into its own file, which can them be imported from other modules without circular issues. The resulting core.py is also much cleaner, and I reorganized the imports so that only three need to be sequentially later in the file (because of complicated imports, again).

Note: because of sequential db changes, this depends on #607 to be merged.

@adelavega adelavega changed the title Add predictior upload route Add predictor upload route Jul 3, 2019
@codecov-io
Copy link

codecov-io commented Jul 3, 2019

Codecov Report

Merging #611 into master will decrease coverage by 1.74%.
The diff coverage is 34.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
- Coverage   72.54%   70.79%   -1.75%     
==========================================
  Files          55       56       +1     
  Lines        2065     2164      +99     
==========================================
+ Hits         1498     1532      +34     
- Misses        567      632      +65
Impacted Files Coverage Δ
neuroscout/resources/__init__.py 100% <ø> (ø) ⬆️
celery_worker/tasks.py 0% <0%> (ø) ⬆️
celery_worker/app.py 0% <0%> (ø) ⬆️
celery_worker/upload.py 0% <0%> (ø)
neuroscout/models/auth.py 96% <100%> (+0.16%) ⬆️
neuroscout/schemas/predictor.py 100% <100%> (ø) ⬆️
neuroscout/schemas/user.py 92.59% <100%> (+0.28%) ⬆️
neuroscout/core.py 67.74% <100%> (-2.85%) ⬇️
neuroscout/models/__init__.py 100% <100%> (ø) ⬆️
neuroscout/populate/ingest.py 92.36% <100%> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7498ec2...b77bd53. Read the comment docs.

@adelavega
Copy link
Collaborator Author

The upload route is working (still need to test Celery task).

The correct request looks like:

curl -X POST "http://localhost/api/predictors/create" -H  "accept: application/json" -H  "authorization: JWT [JWT]" -H  "Content-Type: multipart/form-data" -F "dataset_id=9" -F "runs=258,256" -F "runs=252,251" -F "event_files=@/path/run_1.tsv;type=text/tab-separated-values" -F "event_files=@/path/run_2.tsv;type=text/tab-separated-values" -F "collection_name=test"

@adelavega
Copy link
Collaborator Author

Note the repetition of parameters for runs and event files, but comma separated values for runs

@adelavega
Copy link
Collaborator Author

Upload route is working!

Here is a summary.

  1. Make a POST request to /predictors/create with your JWT.
    The list of runs (and run IDs) must equal the number of files being uploaded.
    Any given run can only be associated with a single event file.
    Event files can have multiple new Predictors, but all of the event files uploaded at once but have the same columns.

Example:

curl -X POST "http://localhost/api/predictors/create" -H  "accept: application/json" 
-H  "authorization: JWT [JWT]" -H  "Content-Type: multipart/form-data" 
-F "dataset_id=9" -F "runs=258,256" -F "runs=252,251" 
-F "event_files=@/path/run_1.tsv;type=text/tab-separated-values" -F "event_files=@/path/run_2.tsv;type=text/tab-separated-values" 
-F "collection_name=test"

Returns:

{
  "collection_name": "test", 
  "id": 13, 
  "predictors": [], 
  "status": "PENDING", 
  "traceback": null, 
  "uploaded_at": "2019-07-05T19:3"
}

This creates a new PredictorCollection which (like NeurovaultCollection or Report), is used to track the Celery status of the upload, and also serves to group together the new predictors that were uploaded. This is also associated with your us id.

To check the status, make a GET request:

curl -X GET "http://localhost/api/predictors/create?id=11" -H "accept: application/json"

which might return something like this if it succeeds:

{
  "collection_name": "test", 
  "id": 11, 
  "predictors": [
    {
      "id": 19244, 
      "name": "reaction"
    }
  ], 
  "status": "OK", 
  "traceback": null, 
  "uploaded_at": "2019-07-05T19:3"
}

You can now use those predictors like normal, and they will display for all users (maybe this is not what we want to do? might want to have this as an option private/public?)

To see which collections you've created do a GET on /api/user with your JWT:

{
  "email": "user@example.com", 
  "first_login": null, 
  "name": null, 
  "picture": null, 
  "predictor_collections": [
    {
      "collection_name": "test", 
      "id": 11
    }
  ]
}

@adelavega adelavega marked this pull request as ready for review July 5, 2019 19:45
@adelavega
Copy link
Collaborator Author

adelavega commented Jul 5, 2019

WDYT? @rwblair

Does this seem reasonable for the frontend?

Minor design quibbles:

  • Maybe this should be under a different route? Maybe it makes more sense to have /predictor_collection/ and do a POST and GET to that? rather than /predictor/create. It would be more "RESTful". OTOH, this is more consistent with how we handle neurovault uploads and reports, so I think it's probably good as is.
  • It might also make more sense to think of "PredictorCollection" is the appropriate name.

To do still:

  • Add tests
  • Check that clearing cache works

@adelavega
Copy link
Collaborator Author

I added some basic tests. Can't fully test because Flask doesn't write out to the test_db, so Celery can't see it.

At a separate time, we should fix this to allow for real testing of Celery tasks.

@adelavega adelavega changed the title Add predictor upload route Add predictor/collection upload route Jul 10, 2019
@adelavega adelavega merged commit 302b37f into master Jul 10, 2019
@adelavega adelavega deleted the enh/upload_pred branch July 10, 2019 19:56
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.

None yet

2 participants