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

Allow resources to be added dynamically to blaze server. #1329

Merged
merged 14 commits into from Dec 8, 2015

Conversation

Projects
None yet
3 participants
@cowlicks
Contributor

cowlicks commented Dec 1, 2015

I'm still having some trouble with this, just getting it out there for now.

closes #1177

@cowlicks

This comment has been minimized.

Contributor

cowlicks commented Dec 2, 2015

API question:

A new data source is added to the severer by passing it a dictionary like {'data_name': data_resource}.

And recall that a blaze server is either instantiated with no data, a single data source, or a dict of data sources.

In the single data case, if I want to add {'foo': bar}, I have no name for my original data. And moving the original data source into a dictionary would change the datashape of the server. What should I name it?

Options:

  • All data on the blaze server must be in a dictionary, so we always have names for separate datasets. This breaks backward compatibility.
  • When adding a new datasource to a new blaze server, move the original data into a dictionary with some arbitrary name that can be learned by asking the server for its datashape.

I think option 1 is sensible.

@cowlicks

This comment has been minimized.

Contributor

cowlicks commented Dec 2, 2015

@cowlicks cowlicks force-pushed the cowlicks:add-to-blaze-server branch 2 times, most recently from a8beb25 to 9889079 Dec 2, 2015

@llllllllll

This comment has been minimized.

Member

llllllllll commented Dec 2, 2015

The server always serves a single resource, the dictionary case is not special it is just that the single resource is itself a record. I could see only supporting the update if the resource is a mutable mapping.

@llllllllll llllllllll added the server label Dec 2, 2015

@cowlicks

This comment has been minimized.

Contributor

cowlicks commented Dec 3, 2015

Anything else y'all'd like to see?

data_not_mm_msg = ("Cannot update blaze server data since its current data"
" is a %s and not a mutable mapping (dictionary like).")
if not isinstance(data, collections.MutableMapping):
return (data_not_mm_msg % type(data), 400)

This comment has been minimized.

@cowlicks

cowlicks Dec 3, 2015

Contributor

Is this the right error code?

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

maybe a 422 Unprocessable Entity would be better here since the syntax of the message is okay:

The 422 (Unprocessable Entity) status code means the server understands the content type of the request entity (hence a 415(Unsupported Media Type) status code is inappropriate), and the syntax of the request entity is correct (thus a 400 (Bad Request) status code is inappropriate) but was unable to process the contained instructions. For example, this error condition may occur if an XML request body contains well-formed (i.e., syntactically correct), but semantically erroneous, XML instructions.

data = _get_data.cache[flask.current_app]
data_not_mm_msg = ("Cannot update blaze server data since its current data"

This comment has been minimized.

@cowlicks

cowlicks Dec 3, 2015

Contributor

How are these messages?

if not isinstance(payload, collections.MutableMapping):
return (payload_not_mm_msg % type(payload), 400)
data.update({(k, resource(v)) for k, v in payload.items()})

This comment has been minimized.

@cowlicks

cowlicks Dec 3, 2015

Contributor

This should be a dict comprehension not a set comprehension.

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

This could also be toolz.valmap(resource, payload). Passing an iterable of tuples to update is the same as passing a dict here though.

)
try:
payload = serial.loads(request.data)

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

since we are just passing k:v pairs, do you think it would make sense to encode this as query params, is there a type of resource that can have a comma in it?

This comment has been minimized.

@cowlicks

cowlicks Dec 3, 2015

Contributor

There is a discussion of this in #1177

I'm still thinking about this.

This comment has been minimized.

@cowlicks

cowlicks Dec 3, 2015

Contributor

Thoughts on this @cpcloud ? I prefer doing this in a POST since it is simpler. But if encoding it as a query is better we should do that.

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

One positive of the post is the ability to send over more complicated structures in the future.

@cowlicks

This comment has been minimized.

Contributor

cowlicks commented Dec 3, 2015

Closes #1177

@cowlicks

This comment has been minimized.

Contributor

cowlicks commented Dec 3, 2015

In #1177 @ywang007 mentions accessing data on the server URL's like http://localhost:6363/cities

Does this work? It is not working for me locally in a web browser.

@llllllllll

This comment has been minimized.

Member

llllllllll commented Dec 3, 2015

There is no data endpoint like that in the default blaze blueprint.

@cowlicks cowlicks changed the title from WIP: allow resources to be added dynamically to blaze server. to Allow resources to be added dynamically to blaze server. Dec 3, 2015

@@ -113,6 +114,32 @@ def authorized(*args, **kwargs):
return authorized
def check_request(f):

This comment has been minimized.

@cowlicks

cowlicks Dec 3, 2015

Contributor

I moved this stuff to a decorator since it was copied in the compserver and addserver functions.

@cowlicks cowlicks added this to the 0.9.0 milestone Dec 3, 2015

@cpcloud

This comment has been minimized.

Member

cpcloud commented Dec 4, 2015

@cowlicks LGTM, can you add a release note under Improved Backends? thx

@cpcloud

This comment has been minimized.

Member

cpcloud commented Dec 4, 2015

@llllllllll any more feedback?

@cpcloud cpcloud added the enhancement label Dec 4, 2015

@cowlicks cowlicks force-pushed the cowlicks:add-to-blaze-server branch 2 times, most recently from ff7e1a7 to 8ac1a42 Dec 4, 2015

@cowlicks

This comment has been minimized.

Contributor

cowlicks commented Dec 4, 2015

Merging soon

@@ -25,6 +25,7 @@ Improved Backends
* Adds support for :func:`~blaze.expr.collections.tail` in the sql backend
(:issue:`1289`).
* Blaze Serever now supports dynamically adding datasets (:issue:`1329`).

This comment has been minimized.

@cpcloud

cpcloud Dec 7, 2015

Member

typo here Serever -> Server

@cpcloud

This comment has been minimized.

Member

cpcloud commented Dec 7, 2015

@cowlicks can you add a doc blurb in the blaze server docs showing a minimal working example?

@cowlicks

This comment has been minimized.

Contributor

cowlicks commented Dec 7, 2015

@cpcloud the current example code with requests in the docs is not working for me. I'm working on it.

@cowlicks cowlicks force-pushed the cowlicks:add-to-blaze-server branch from 8ac1a42 to 196d7c4 Dec 7, 2015

@@ -324,6 +324,22 @@ These queries deconstruct the Blaze expression as nested JSON. The ``":leaf"``
string is a special case pointing to the base data. Constructing these queries
can be difficult to do by hand, fortunately Blaze can help you to build them.
Adding Data to the Server

This comment has been minimized.

@cowlicks

cowlicks Dec 7, 2015

Contributor

@cpcloud doc blurb

@@ -309,7 +309,7 @@ server::
}
$ curl \
-H "Content-Type: application/json" \
-H "Content-Type: application/vnd.blaze+json" \

This comment has been minimized.

@cowlicks

cowlicks Dec 7, 2015

Contributor

@llllllllll I changed all of these.

data2 = data.copy()
data2.update({'iris': resource(iris_path)})
expected2 = str(discover(data2))
assert response2.data.decode('utf-8') == expected2

This comment has been minimized.

@llllllllll

llllllllll Dec 7, 2015

Member

can we check the dshape before adding and then compare it to the dshape after adding the new resource?

This comment has been minimized.

@cowlicks

cowlicks Dec 7, 2015

Contributor

Yeah. This caught a bug with the tests actually. I'm mutating the global server, since this gets run with multiple formats we are updating the server with data it already has.

headers=mimetype(serial),
data=blob,
)
assert 'OK' in response1.status

This comment has been minimized.

@llllllllll

llllllllll Dec 7, 2015

Member

we can do: assert response1.status_code == 200

headers=mimetype(serial),
data=blob,
)
assert '422' in response1.status

This comment has been minimized.

@llllllllll

llllllllll Dec 7, 2015

Member

we can also check the status_code here

This comment has been minimized.

@cowlicks

cowlicks Dec 7, 2015

Contributor

👍

cowlicks added some commits Dec 7, 2015

cowlicks added a commit that referenced this pull request Dec 8, 2015

Merge pull request #1329 from cowlicks/add-to-blaze-server
Allow resources to be added dynamically to blaze server.

@cowlicks cowlicks merged commit 43d2f7e into blaze:master Dec 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment