DM-9034: Upload full validate_base blobs, measurement metadata and metric definitions #7

afausti · 2017-01-31T15:59:59Z

Changes in this PR include upload of data blobs produced by validate_drp job, measurement metadata and metric definitions to SQUASH API

- metric definition from validate_drp is uploaded to SQUASH in a separate request - renamed squash.json to job.json

- added blobs and measurement metadata to job json document as expected by SQUASH api/jobs endpoint - shim metrics definition to SQUASH api/metrics endpoint, check for registered metrics and add new metrics only

- --api-url option now expects the SQUASH API root url instead of the jobs endpoint - new function to load registered metrics from SQUASH - upload metrics definition to SQUASH - since metrics definition are uploaded dynamically the --metrics (accepted metrics) option is no longer required

jonathansick · 2017-01-31T16:20:53Z

@afausti Looks like you'll need to fix the schema loader import in https://github.com/lsst-sqre/post-qa/blob/tickets/DM-9034/tests/test_cli.py#L12

afausti · 2017-02-01T18:56:41Z

@jonathansick I fixed most of the tests after the changes in this PR, but couldn't make the test_upload_json work. If you run the tests the error is:

ConnectionError: Connection refused: GET https://squash.lsst.codes/dashboard/api/jobs

I don't understand how do you mock the api to make the json upload test.

afausti · 2017-02-01T18:59:11Z

setup.py

@@ -18,6 +18,7 @@ def read(filename):
        filename)
    return open(full_filename, mode='r', encoding='utf-8').read()

+


It seems that flake8 behaves differently when I run it locally, this commit can probably be ignored.

jonathansick · 2017-02-06T17:46:14Z

@afausti I see the problem with test_upload_json. Essentially we have new http requests happening, and we need to create mocks for each request. You can see how the original POST was mocked:

    responses.add(responses.POST, 'https://squash.lsst.codes/api/jobs',
                  body='{}', status=201,
                  content_type='application/json')

And now we need to add another responses.add for each of the new GETs that happen in upload_json_doc and load_registered_metrics:

GET https://squash.lsst.codes/dashboard/api
GET https://squash.lsst.codes/dashboard/api/metrics (or whatever is happening in L146 of cli.py upload_json_doc.

And also add one for the new POST:

POST https://squash.lsst.codes/dashboard/api/metrics

Note that in responses.add the body is needs to be the expected JSON return document that that HTTP request.

I can help you set up all of these mocks, but first we'd need to write down all the expected JSON responses and status codes (I don't know these yet).

…ered_metrics()

afausti · 2017-02-07T04:40:35Z

@jonathansick thanks for the guidance in using responses, tests are passing now.

jonathansick · 2017-02-07T04:56:36Z

👏 I'll do a proper review by tomorrow

jonathansick

Overall this looks really great. I like the test coverage and the validation. I have some questions/suggestions, particularly about building a new metric json upload—see line comments.

Also, I know you reverted the change, but in my local environment I'm also seeing an E305 error at setup.py line 21 (needs two blank lines). Could you try putting this in?

(Looks like there could be an issue with flake8 in the CI)

jonathansick · 2017-02-07T17:24:30Z

postqa/jsonshim.py

+        'parameters': metric['parameters'],
+        # Unit is being stored as part of the metric specification in
+        # SQuaSH
+        'unit': vdrp_measurement_doc['unit']


I'd recommend add a FIXME comment here pointing to DM-9312.

agreed, thanks for opening the ticket.

jonathansick · 2017-02-07T17:27:16Z

tests/data/api_response.json

+  "metrics": "http://squash.lsst.codes/dashboard/api/metrics/",
+  "measurements": "http://squash.lsst.codes/dashboard/api/measurements/",
+  "datasets": "http://squash.lsst.codes/dashboard/api/datasets/"
+}


GitHub is worried that there's no newline at the end of this file.

oops, will fix that.

jonathansick · 2017-02-07T17:50:30Z

postqa/jsonshim.py

        if vdrp_measurement_doc['value'] is None:
            continue
+        if vdrp_measurement_doc['metric']['name'] not in registered_metrics:
+            metrics.append(shim_metric_definition(vdrp_measurement_doc))


Is it possible for a metric to appear multiple times in this list? Like, if there's multiple AM1 measurements in different bands, or multiple measurements against different specifications of a metric, then couldn't we have duplicates here?

That's a good point. In this implementation the metric name is unique so if there is already an AM1 registered it would not pick another AM1 with a different specification.

I see the problem when a metric, say AD1, depends on another metric specification, in this case AF1. We end up with three measurements of AD1 for each AF1 specification level, right?

I think if a metric has a different parameter or specification it should be named differently, so that we can identify each metric uniquely by its name, or perhaps have another mechanism like a composite primary key based on the metric name and specification? we should think on how to display the metric measurements in this situation too.

I propose to defer this issue to another ticket in order to close this one. Opened https://jira.lsstcorp.org/browse/DM-9330

jonathansick · 2017-02-07T17:53:11Z

postqa/jsonshim.py

        measurements.append(shim_vdrp_measurement(vdrp_measurement_doc))
+
+    metric_json = metrics


Maybe rename metrics to new_metrics_json from the outset so it's clear these are metric's we'll be uploading and avoids having to rename the variable here.

jonathansick · 2017-02-07T17:57:30Z

postqa/cli.py

-        upload_json(job_json, api_url=args.api_url,
-                    api_user=args.api_user, api_password=args.api_password)
+
+        upload_json_doc(metric_json, api_url=args.api_url,


Do we (or should we) skip this upload if there are no new metrics, beyond what's in registered_metrics?

Indeed it does a POST request of an empty json doc, I can improve that.

jonathansick · 2017-02-07T18:00:06Z

postqa/cli.py

+    return registered_metrics
+
+
+def upload_json_doc(json_doc, api_url, api_endpoint, api_user=None, api_password=None):


I think this line is too long; maybe wrap at api_user?

jonathansick · 2017-02-09T17:33:16Z

@afausti I've push a commit with the Travis set up for PyPI deployments. Make sure you git pull to refresh your ticket branch.

Here's the deployment procedure:

Update version in setup.py. This might warrant a v1.2.0.
Add to CHANGELOG.rst what the changes are
Commit these changes, saying "Bump to v1.2.0"
git tag -a v1.2.0 -m "v1.2.0"
git push and git push --tags
merge your ticket branch to master
💰

jonathansick

Great, see my comment for the release procedure.

afausti added 5 commits January 31, 2017 06:44

Added schema to validate SQUASH api/metrics endpoint

d58adfd

- metric definition from validate_drp is uploaded to SQUASH in a separate request - renamed squash.json to job.json

Renamed file after adding metrics schema

2fb3bfd

Make function to load schema more generic

2c1429a

Changes in JSON documents uploaded to SQUASH

eab8368

- added blobs and measurement metadata to job json document as expected by SQUASH api/jobs endpoint - shim metrics definition to SQUASH api/metrics endpoint, check for registered metrics and add new metrics only

afausti added 4 commits February 1, 2017 11:42

Removed load_schema call inside validate to simplify tests

38c0eb3

registered_metrics can be empty by default to simplify tests

b5b2a5e

Fix tests

c6fa68a

PEP8 cleanup

47e8f50

afausti commented Feb 1, 2017

View reviewed changes

jonathansick changed the title ~~Tickets/dm 9034~~ DM-9034: Upload full validate_base blobs, measurement metadata and metric definitions Feb 6, 2017

afausti added 2 commits February 6, 2017 21:33

Fixed test for upload_json_doc() and added a new test for load_regist…

84a0032

…ered_metrics()

Reverted previous commit, flake8 behaves differently locally

d9ecee0

jonathansick suggested changes Feb 7, 2017

View reviewed changes

afausti and others added 2 commits February 9, 2017 06:51

Addressed PR comments

6823b0f

Enable PyPI deploy from Travis when tagging

3ef335f

jonathansick approved these changes Feb 9, 2017

View reviewed changes

Bump to v1.3.0

a917b17

afausti merged commit 7ae5218 into master Feb 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-9034: Upload full validate_base blobs, measurement metadata and metric definitions #7

DM-9034: Upload full validate_base blobs, measurement metadata and metric definitions #7

afausti commented Jan 31, 2017

jonathansick commented Jan 31, 2017

afausti commented Feb 1, 2017

afausti Feb 1, 2017

jonathansick commented Feb 6, 2017

afausti commented Feb 7, 2017

jonathansick commented Feb 7, 2017

jonathansick left a comment

jonathansick Feb 7, 2017

afausti Feb 8, 2017

jonathansick Feb 7, 2017

afausti Feb 8, 2017

jonathansick Feb 7, 2017

afausti Feb 8, 2017

afausti Feb 8, 2017

jonathansick Feb 7, 2017

afausti Feb 8, 2017

jonathansick Feb 7, 2017

afausti Feb 8, 2017

jonathansick Feb 7, 2017

afausti Feb 8, 2017

jonathansick commented Feb 9, 2017

jonathansick left a comment

		@@ -18,6 +18,7 @@ def read(filename):
		filename)
		return open(full_filename, mode='r', encoding='utf-8').read()

		measurements.append(shim_vdrp_measurement(vdrp_measurement_doc))

		metric_json = metrics

		return registered_metrics


		def upload_json_doc(json_doc, api_url, api_endpoint, api_user=None, api_password=None):

DM-9034: Upload full validate_base blobs, measurement metadata and metric definitions #7

DM-9034: Upload full validate_base blobs, measurement metadata and metric definitions #7

Conversation

afausti commented Jan 31, 2017

jonathansick commented Jan 31, 2017

afausti commented Feb 1, 2017

Choose a reason for hiding this comment

jonathansick commented Feb 6, 2017

afausti commented Feb 7, 2017

jonathansick commented Feb 7, 2017

jonathansick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathansick commented Feb 9, 2017

jonathansick left a comment

Choose a reason for hiding this comment