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

Added `delete_tileset` manage command and `tileset` API route #79

Closed
wants to merge 6 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@alexpreynolds
Copy link
Contributor

alexpreynolds commented Nov 13, 2018

Here is a demonstration tileset:

$ python manage.py list_tilesets
tileset: Tileset [name: LN43287.75_20.normalized.GRCh38_no_alts.bw.tileset] [ft: hitile] [uuid: AIVpsJYwSemD8FVPBv6vrw]
$ ls -al higlass-server/media/uploads/
total 207764
drwxrwxr-x 2 ubuntu ubuntu      4096 Nov 13 06:15 ./
drwxrwxr-x 3 ubuntu ubuntu      4096 Nov 13 06:15 ../
-rw-rw-r-- 1 ubuntu ubuntu 212742028 Nov 13 06:15 LN43287.75_20.normalized.GRCh38_no_alts.bw.tileset

Here is an example showing the use of the manage.py delete_tileset command to remove this tileset file and database record:

$ python manage.py delete_tileset --uuid=AIVpsJYwSemD8FVPBv6vrw
$ python manage.py list_tilesets
$ ll higlass-server/media/uploads/
total 8
drwxrwxr-x 2 ubuntu ubuntu 4096 Nov 13 06:52 ./
drwxrwxr-x 3 ubuntu ubuntu 4096 Nov 13 06:15 ../

The API call uses the DELETE method and the IsAuthenticated permission class decorator, which requires authentication. Here is an example of an (unauthenticated) API call:

$ python manage.py list_tilesets
tileset: Tileset [name: LN43287.75_20.normalized.GRCh38_no_alts.bw.tileset] [ft: hitile] [uuid: MSLMji4TSVKynUGNui-5cQ]
...
$ curl -X "DELETE" http://localhost:8000/api/v1/tileset/?d=MSLMji4TSVKynUGNui-5cQ
{"uuid": "MSLMji4TSVKynUGNui-5cQ"}

The API returns 40x errors if the underlying instance or file is not found or cannot be removed.

For debugging purposes, I modified the Tileset model __str__ function to return the tileset's uuid along with its name and filetype properties.

@alexpreynolds

This comment has been minimized.

Copy link
Contributor Author

alexpreynolds commented Nov 13, 2018

I added a management command and API route to modify a tileset's properties by uuid. Currently, this only modifies the name field (if specified), but this could be expanded to modify other tileset properties with additional logic.

Here is an example management command:

$ python manage.py modify_tileset --uuid="Zfg_ZtU0TZeJYRoRQwRMFQ" --name="bar"

And here is an example API call:

$ curl -H "Accept: application/json" -X "POST" -d '{"uuid":"BAeHd65pSCmI8Cs-xS5ACw", "name":"foo"}' http://localhost:8000/api/v1/tileset/

alexpreynolds and others added some commits Nov 13, 2018

@pkerpedjiev

This comment has been minimized.

Copy link
Contributor

pkerpedjiev commented Nov 13, 2018

This is great! Thanks Alex.

A few things before merging:

  1. As mentioned in the inline comments, could you check if the DELETE endpoint doesn't already exist in TilesetsViewSet?
  2. The documentation for higlass_server is here: https://github.com/higlass/higlass/blob/develop/docs/higlass_server.rst. Would you mind adding a few lines describing the new API endpoints?
  3. It looks like you've tested this quite extensively. To make sure we don't introduce regressions, could you just copy those manual tests to our automated test section here: https://github.com/higlass/higlass-server/blob/master/higlass_server/tests.py?

Thanks!

@@ -492,6 +494,95 @@ def tiles(request):
)

return JsonResponse(tiles_to_return, safe=False)


@api_view(['POST', 'DELETE'])

This comment has been minimized.

@pkerpedjiev

pkerpedjiev Nov 13, 2018

Contributor

Did you check if the curl -X 'DELETE' server:port/api/v1/tilesets/[uuid]/ endpoint already exists? There's already a tilesets viewset that should provide, GET, POST, UPDATE, DELETE endpoints here:

class TilesetsViewSet(viewsets.ModelViewSet):

I know the POST endpoint works because I've been using it to upload datasets to higlass.io. Take a look here: http://docs-dev.higlass.io/data_preparation.html#importing-into-higlass

If possible, I'd prefer to avoid duplicating API endpoints.

@@ -55,6 +55,34 @@ curl http://localhost:8000/api/v1/tileset_info/?d=hitile-demo
curl http://localhost:8000/api/v1/tiles/?d=hitile-demo.0.0.0
```

## Preparing files for ingest

This comment has been minimized.

@pkerpedjiev

pkerpedjiev Nov 13, 2018

Contributor

All of the datatype specific documentation is in the main documentation repository: http://docs-dev.higlass.io/data_preparation.html

The code for that can be found here: https://github.com/higlass/higlass/tree/develop/docs

@@ -1,8 +1,8 @@
Cython==0.25.2
-e git+https://github.com/mirnylab/cooler.git@master#egg=cooler

This comment has been minimized.

@pkerpedjiev

pkerpedjiev Nov 13, 2018

Contributor

What's in the master branch that isn't in v0.7.10?

@alexpreynolds

This comment has been minimized.

Copy link
Contributor Author

alexpreynolds commented Nov 15, 2018

Sorry. I'm going to close this PR and start over.

@pkerpedjiev

This comment has been minimized.

Copy link
Contributor

pkerpedjiev commented Nov 15, 2018

No, it's my fault that we don't have better documentation about the API endpoints. Ideally the docs would have a section on the REST API. Without looking at the code it's nearly impossible to figure out what endpoints exist and how to access them.

All this is to say, your PR is really appreciated and hopefully it's not too much work to fit it into TilesetViewSet.

@alexpreynolds

This comment has been minimized.

Copy link
Contributor Author

alexpreynolds commented Nov 15, 2018

I neglected to sync my fork with upstream, so starting over was necessary, anyway.

Looks like the built-in DELETE does remove the database entry, but it does not remove the tileset file from the media/uploads directory. I'll look at overriding the behavior of the destroy() method to take care of that, so that it would behave the same as the management command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment