Parse args for features / datasources of interest #115
Conversation
|
|
||
| params = {} | ||
| if request.method == "POST": | ||
| data = request.get_json() # Assume POSTs are JSON. |
There was a problem hiding this comment.
Why not just let POST data be x-www-form-urlencoded and use the same code to parse?
There was a problem hiding this comment.
People like JSON? Not sure.
We could add a check for request.headers.get("Content-Type") == "application/json" and accept all three?
There was a problem hiding this comment.
Seems like a lot of complication. The cache can't have a hierarchy to it. It's only one-dimensional. Since JSON object keys must be strings, I'm not sure if we get any benefit from JSON here. We'd be implementing JSON-soap.
|
Whoops, didn't mean to close that. |
| # Is this right? What's a precache? | ||
| precache = ("precache" in request.args) and (len(params) == 0) | ||
|
|
||
| # FIXME: Do something with `params`. |
There was a problem hiding this comment.
scores_processor.score(content, model, [rev_id], cache=params, precache=precache)
|
Sorry for the delay here. I'm working on a write-up of the different ways we can have users provide dependencies for feature extraction for feedback. Should have an update this weekend. |
|
Sure, no rush. Enjoy your weekend :) |
|
I got a write-up together of what I see as the two good options we have. See #101 (comment) |
|
Thanks, taking a look. |
| # Parse values for features / datasources of interest. | ||
| try: | ||
| for k, v in request.values.items(): | ||
| if k == "caches": |
There was a problem hiding this comment.
In the discussion, you said cache but argument to score() is caches.
There was a problem hiding this comment.
Ahh yes. That's right. I was imagining that we'd only support cached value injection on the /<context>/<model>/<revid>/ route where caches would be a simple {<revid>: { ... our flat JSON ... }}.
|
Updated for |
| @@ -61,3 +63,18 @@ def format_output(context, scores, model_info, warning=None, notice=None): | |||
| for rev_id in scores[model]: | |||
| output['scores'][context][model]['scores'][rev_id] = scores[model][rev_id] | |||
| return jsonify(output) | |||
|
|
|||
|
|
|||
| # Parse values for features / datasources of interest. | |||
There was a problem hiding this comment.
you should put documents in docstrings after function definition
There was a problem hiding this comment.
Ok, but there are number of other PEP8 warnings in the library. Should they be fixed? Should CI not pass until they are?
There was a problem hiding this comment.
I'll fix all of PEP8 issues in another PR, don't worry. It'll pass without this but I'm saying you're providing documentation, do it in the most proper way.
There was a problem hiding this comment.
Good Q. Don't worry about pep 8 issues on the rest of the file structure (unless you want to). We can get those in a follow up PR.
|
This latest build failure seems unrelated. |
|
Or not, nevermind, let me fix that. |
* Introduced in 14f6c09
|
Should be good to go now. |
| @@ -70,7 +70,7 @@ def extract_roots(self, model, rev_ids, caches=None): | |||
| root_ds = [d for d in dependencies.dig(features) | |||
| if isinstance(d, Datasource)] | |||
| error_root_vals = self.extractor.extract(rev_ids, root_ds, | |||
| cache=caches) | |||
| caches=caches) | |||
There was a problem hiding this comment.
This doesn't match the behavior of revscoring
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
OK fair point. I'm just going to fix this in revscoring now. Will have a PR shortly.
There was a problem hiding this comment.
You'll also need to make a release and bump the requirements.txt here.
|
This was merged, but I had to rebase on the way so it seems to have not been caught. Thanks for your work, @arlolra. We'll be deploying this soon. I'll let you know. |
I see. Thanks @halfak! |
A start on #101
Some questions inline.