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

initial REST api #18

Merged
merged 19 commits into from
Jun 26, 2017
Merged

initial REST api #18

merged 19 commits into from
Jun 26, 2017

Conversation

jan-xyz
Copy link
Contributor

@jan-xyz jan-xyz commented Jun 2, 2017

This pull request adds the capability to have a JSON REST API discussed in #15 . I see it as a first step towards better URLs.

It doesn't touch upon the existing functionality. The only thing that was touched from the old code is the way that temporary directories for input and output files are created. They are no longer per visualisation request but per session with a session lasting a default of 31 days.

The functionality can be used with something like curl from the CLI or scripts/programs.

There are 4 new URLs that can be used:
GET /api/: right now it is only a placeholder. It could potentially be used to start a session to authenticate user and to display the API version.
cmd: curl "localhost:5000/api/"
output:

{ "hello": "world" }

POST /api/images: is used to upload images. It returns the filename and a UID in JSON
cmd: curl -F "file=@/path/to/image.png" localhost:5000/api/images -b /path/to/cookie -c /path/to/cookie
output:

{  "file": "image.png",  "ok": "true",  "uid": 0 } 

GET /api/images: lists all images uploaded via this API
cmd: curl "localhost:5000/api/images" -b /path/to/cookie -c /path/to/cookie
output:

{
  "images": [
    {
      "filename": "Screen_Shot_2016-11-08_at_22.57.51.png",
      "uid": 0
    },
    {
      "filename": "Image.png",
      "uid": 1
    }
  ]
}

GET /api/visualize: this page need to at least 2 arguments image=X and visualizer=X.
cmd: curl "localhost:5000/api/visualize?image=0&visualizer=PartialOcclusion" -b /path/to/cookie -c /path/to/cookie
output:

{
  "output": [
    {
      "example_filename": "1496440342.3700328Image.png",
      "input_filename": "Image.png",
      "predict_probs": [
        {
          "index": 2,
          "name": "2",
          "prob": "0.769"
        },
        {
          "index": 8,
          "name": "8",
          "prob": "0.133"
        },
        {
          "index": 3,
          "name": "3",
          "prob": "0.064"
        },
        {
          "index": 7,
          "name": "7",
          "prob": "0.012"
        },
        {
          "index": 5,
          "name": "5",
          "prob": "0.009"
        }
      ],
      "result_filenames": [
        "1496440342.43444780_Image.png",
        "1496440342.6356451_Image.png",
        "1496440342.8196582_Image.png",
        "1496440343.0056613_Image.png",
        "1496440343.1946724_Image.png"
      ]
    }
  ]
}

All files referenced in the API can be directly accessed via the already existing URLs:
/inputs/<filename> and /outputs/<filename>

EDIT: I changed the described API to represent the latest state of the branch which includes a consolidated path for listing and posting images which is according to the canonical way for resource management in REST and having the files being stored per session and not globally.

@codecov-io
Copy link

codecov-io commented Jun 2, 2017

Codecov Report

Merging #18 into master will increase coverage by 0.37%.
The diff coverage is 96.49%.

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   92.45%   92.82%   +0.37%     
==========================================
  Files          11       11              
  Lines         464      516      +52     
==========================================
+ Hits          429      479      +50     
- Misses         35       37       +2

@rhsimplex
Copy link
Contributor

@jan-xyz this looks great, thanks! I will take me a bit of time to review, I'll update you soon.

@rhsimplex
Copy link
Contributor

Yeah this looks like the way to go. I think we'll still need to start a session because otherwise a user calling e.g. api/list can see all other users' file uploads.

Is there a particular reason why you direct all files to a single folder instead of on a per-request basis? Will this result in filename clashes even with secure_filename? (And what happens to the files? The current code also doesn't handle file lifecycle, so I made a separate issue #19).

We're on the right track though. When you finish this, it should make building a real front-end much, much easier.

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jun 7, 2017

Regarding file storage/lifecycle: I imagine the whole thing to have an authentication mechanism in the future, including a guest mode. That would take care of file storage as well as lifecycle. You could possibly limit the storage space per user or leave it unlimited, and the guest just can upload one image at a time that gets overwritten by the next upload.

I like the idea of reusing the same file for different visualisations, and it feels like a lot of overhead to re-upload these files over and over again. So, if we do it per-request we will certainly reduce the complexity but remove the possibility to re-use files. I am fine with both.

All secure_filename does is sanitising the input following the mantra never trust the user. I did not include any file duplication or clash_avoidance in this PR.

@rhsimplex
Copy link
Contributor

I agree, it makes more sense. Can you create the folders on a per-session basis and still get the same behavior?

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jun 8, 2017

I don't see why this shouldn't be possible. I'll look into it and update the PR accordingly :)

@rhsimplex
Copy link
Contributor

Hey @jan-xyz was out of town this weekend but saw the new commits coming in. Give me some time to look over it and I'll get back to you soon!

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jun 13, 2017

No problem at all. I am looking forward to it. Note though that cookie/session based handling of the state is against REST as it means that the API isn't stateless. Once this is merged I would like to start working on a follow-up that brings it more in line with what REST means. That is:

  1. A user-based upload/access for advanced usage
  2. A guest mode that consists of only one step
    • e.g. /api/upload_and_visualize?visualizer=Vis&Param1=50&Param2=XYZ and an image POSTed in the body

That way we will be back on track for REST and it allows us to properly determine the life cycle of the files as well as separate users. The file used in the guest mode will be cleaned up directly after the request, the users will have a configurable amount of files they can upload and re-use

@rhsimplex
Copy link
Contributor

Hey @jan-xyz, a bit embarrassing but I'm having trouble getting the curl commands to work with cookies. Are you pointing to an existing cookie path generated from the browser, or just any file or what? I'm going through the functionality at the moment.

And it's out of the scope of this PR, but maybe we should have custom 404 etc responses (right now it just dumps the raw html) in a future PR =)

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jun 20, 2017

Sorry, I didn't update the examples. I brought the API more in line with the canonical way in a commit. Curl writes its own cookie and you can specify any cookie (e.g. I use ./cookie).

example:

Ava:~ jan$ curl "localhost:5000/api/images" -b ./cookie -c ./cookie
{
  "images": []
}
Ava:~ jan$ curl -F "file=@/Users/jan/Desktop/Image.png" localhost:5000/api/images -b ./cookie -c ./cookie
{
  "file": "Image.png",
  "ok": "true",
  "uid": 0
}
Ava:~ jan$ curl -F "file=@/Users/jan/Desktop/Image.png" localhost:5000/api/images -b ./cookie -c ./cookie
{
  "file": "Image.png",
  "ok": "true",
  "uid": 1
}
Ava:~ jan$ curl "localhost:5000/api/images" -b ./cookie -c ./cookie
{
  "images": [
    {
      "filename": "Image.png",
      "uid": 0
    },
    {
      "filename": "Image.png",
      "uid": 1
    }
  ]
}

@rhsimplex
Copy link
Contributor

rhsimplex commented Jun 22, 2017

I pushed a couple changes to your PR. If you think they are good, go ahead and merge them and if not let me know!

After that, I'll check it for code formatting/style and we can merge.

Once again, great work =)

improve handling of session
@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jun 22, 2017

Wow, learned something new about flask again! :) Cheers for the improvement, I already thought it's weird to check the session before every request but couldn't come up with a better way. That decorator is really helpful!

@rhsimplex
Copy link
Contributor

Ok just pushed a couple small additions. The only thing missing now is api_visualize will need to take multiple image ids (it's much faster to pass all inputs to the backend than one at a time). We can do this now, or make another PR after we merge.

Then I see three things that need to be done (after the merge).

  1. API Documentation
  2. picasso.py refactor. The API functions should be in api.py and the app fucntions in views.py or something like that. The tests should also be split up into one class per file.
  3. Rewrite the web app to use the API calls. This is the fun one =)

We'd be more than grateful if you want to continue working on these parts or anything else. Let me know your thoughts!

doc strings and improved api message
@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jun 24, 2017

I would suggest to put these all into separate PRs. This one is getting quite large now with 162 additions.

@rhsimplex rhsimplex merged commit 4878f96 into merantix:master Jun 26, 2017
@jan-xyz jan-xyz deleted the rest_api branch June 27, 2017 12:14
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.

3 participants