[KED-2035] Expose parameter metadata #275

921kiyo · 2020-10-06T12:01:06Z

Description

As an extension for metadata side panel, we are exposing parameters metadata to Viz. Here are examples of the endpoints and and outputs.

http://127.0.0.1:4141/api/nodes/d577578a (endpoint for params:xxx)

{
  "parameters": 0.2
}

http://127.0.0.1:4141/api/nodes/f1f1425b (endpoint for parameters)

{
  "parameters": {
    "example_learning_rate": 0.01,
    "example_num_train_iter": 10000,
    "example_test_data_ratio": 0.2
  }
}

Development notes

QA notes

Modified unit tests

Checklist

Read the contributing guidelines
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added new entries to the RELEASE.md file
Added tests to cover my changes

Legal notice

I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

richardwestenra · 2020-10-07T09:43:38Z

This is great, thank you! So if I were to click on a single parameter node on the chart, which type of endpoint would I get? I assume it'd be the single params:xxx one, right? Can you please give a bit more info about the difference between these two?

921kiyo · 2020-10-07T09:52:39Z

This is great, thank you! So if I were to click on a single parameter node on the chart, which type of endpoint would I get? I assume it'd be the single params:xxx one, right? Can you please give a bit more info about the difference between these two?

Yes! In Kedro side, there are 2 ways to define parameters. Let's say in my parameters.yml in Kedro side, I have the following parameter definition.

example_test_data_ratio: 0.2
example_num_train_iter: 10000
example_learning_rate: 0.01

If I want to access all of them in one of my Kedro nodes, I can use a keyword parameters, which returns all of them as a dictionary like

"parameters": {
    "example_learning_rate": 0.01,
    "example_num_train_iter": 10000,
    "example_test_data_ratio": 0.2
  }

But image if I have 1000 parameters in my parameters.yml, I probably don't want to grab all of them at once. If I only need to access just one of them, I can use params:example_test_data_ratio

From Kedro Viz UI, both params:xxx and parameters show up as parameter nodes

Hope this helps :)

limdauto

👏

richardwestenra · 2020-10-07T13:07:50Z

ah okay interesting, thanks! Looks like you can access them both from clicking nodes on the graph then. It'd be great for these to have the same structure if possible, but I can't think of a way I'd do it better. I guess when receiving the data, we can just use typeof parameters === 'object', as long as the content of the single-param property parameters will only ever be either an object literal or a number/string/boolean. If it's possible for it to be something else (e.g. an array or object literal) then maybe we should use a different structure.

To explain better what I mean:

// params:xxx
{
  // these are fine:
  "parameters": 0.2
  "parameters": 'this is a string'
  "parameters": true
  // these would cause problems, because I can't distinguish them from the full dictionary:
  "parameters": [0, 1, 2, 3]
  "parameters": { a: 1, b: 2 }
}

The more I think about it, the more I think that this structure is running too many risks in assuming that these are distinguishable. If we want to be more clear about distinguishing them, maybe we should use a different property name, e.g. parameter vs parameters?

However you could also make the case that they just don't need to be distinguished, and then we can just display whatever the content is? Depends whether we should just display multiple parameters as code, or separate them and design them differently

921kiyo · 2020-10-07T14:03:48Z

To give you more example, parameters can be nested.
If I have the following paramter definitions in parameters.yml,

example_test_data_ratio:
  nested1: 0.2
  nested2: 0.3
  nested3: 0.4
example_num_train_iter: 10000
example_learning_rate: 0.01

Hitting params:example_test_data_ratio will return

{
  "parameters": {
    "nested1": 0.2,
    "nested2": 0.3,
    "nested3": 0.4
  }
}

and parameters endpoint will return

{
  "parameters": {
    "example_learning_rate": 0.01,
    "example_num_train_iter": 10000,
    "example_test_data_ratio": {
      "nested1": 0.2,
      "nested2": 0.3,
      "nested3": 0.4
    }
  }
}

Viz UI will look the same as above screenshot (just metadata is nested)

richardwestenra · 2020-10-07T20:17:30Z

@921kiyo cool in that case, as mentioned on Slack, I propose that for single params, you just include a single key/value pair. instead of this:

{
  parameters: 0.01,
}

do this instead:

{
  parameters: {
    "example_learning_rate": 0.01,
  }
}

So that way, the formatting is consistent and we can treat them the same way.

package/kedro_viz/server.py

RELEASE.md

package/kedro_viz/server.py

DmitriiDeriabinQB · 2020-10-12T12:22:06Z

package/kedro_viz/server.py

+        # In case of 'params:' prefix
+        parameters_metadata = {
+            "parameters": {
+                next(iter(parameters)): next(iter(parameters.values())).load()


The readability is not great here. Can you reduce the nesting by doing

key, value = next(iter(parameters.items()))

DmitriiDeriabinQB · 2020-10-12T12:57:52Z

package/kedro_viz/server.py

-    # return empty JSON for parameters type
-    return jsonify({})
+    parameters = node["obj"]
+    if isinstance(parameters, dict):


I'm not a big fan of such isinstance here. Can we maybe explicitly save parameter name in the dict we put in _JSON_NODES?
Something like:

if "parameter_name" in node: # handle "params:..." else: # handle "parameters"

@limdauto wdyt?

Yeah, I could do that but how do you get "parameter_name" in this function? we can only access to parameter_name from next(iter(parameters.items())) correct?

I meant I don't particularly like the fact that instead of the object we put the dictionary with name and object. Parameter name can have its own top-level key rather than being squeezed into the obj key.

@DmitriiDeriabinQB Hope I understand you correctly, but it sounds like you're asking for the single-parameter endpoint to have a different structure from the multiple-parameter endpoint, is that right? Kiyo had it like that originally, but I asked him to change it.

If you think about this from the front-end perspective, we might want to separate out different keys to display them differently in the viz meta panel when clicking on a 'Parameters' node - e.g.:

but if you click on a single node, we might want to display it the same way, but if it's not in the same format we might not be able to recognise it as a distinct type just from the structure, because a parameter can be one of lots of different types. So we're using the same structure to keep it simple on the front end. Does that help? Hope I haven't misunderstood you

@richardwestenra I'm still keeping the format you asked, his suggestion was more about how to generate it in code :)

@richardwestenra yeah, I read through your discussion with Kiyo above and understand the solution. I wasn't talking about the format it is exposed to the frontend but rather the way the data stored in the backend. I'm moderately cautious about the complexity of the data wrangling logic on Python side since the data itself is quite trivial.
TLDR: It's purely the discussion about the Python side of things and should not affect the API contract.

Co-authored-by: Dmitrii Deriabin <[email protected]>

limdauto · 2020-10-12T16:18:11Z

package/kedro_viz/server.py

+    if "parameter_name" in node:
+        # In case of 'params:' prefix
+        parameters_metadata = {
+            "parameters": {node["parameter_name"]: node["obj"].load()}


Sorry could you remind me what does node["obj"].load() do?

node["obj"] is MemoryDataSet, so it will load the value of params:key parameter (e.g. 0.2)

DmitriiDeriabinQB · 2020-10-12T17:59:27Z

package/kedro_viz/server.py

@@ -539,8 +541,15 @@ def nodes_metadata(node_id):
        dataset_metadata = _get_dataset_metadata(node)
        return jsonify(dataset_metadata)

-    # return empty JSON for parameters type
-    return jsonify({})
+    if "parameter_name" in node:


This is much better (to me personally at least), thanks @921kiyo 👍

package/kedro_viz/server.py

921kiyo added 4 commits October 6, 2020 11:23

get parameter metadata

835eabc

fix tests

25bdd85

trigger

b31514e

backward compatibility

66d8104

921kiyo requested a review from DmitriiDeriabinQB as a code owner October 6, 2020 12:01

921kiyo self-assigned this Oct 6, 2020

update release note

1ce451b

921kiyo requested review from richardwestenra and yetudada as code owners October 6, 2020 12:02

921kiyo requested review from limdauto and removed request for yetudada October 6, 2020 12:02

limdauto approved these changes Oct 7, 2020

View reviewed changes

update params:key

21fb9af

DmitriiDeriabinQB reviewed Oct 8, 2020

View reviewed changes

package/kedro_viz/server.py Outdated Show resolved Hide resolved

package/kedro_viz/server.py Outdated Show resolved Hide resolved

richardwestenra changed the title ~~Expose parameter metadata~~ [KED-2035] Expose parameter metadata Oct 8, 2020

921kiyo added 2 commits October 8, 2020 10:49

reformat output for params: prefix

cfa5703

Remove node_id args

90941d0

921kiyo added the hacktoberfest-accepted label Oct 8, 2020

fix typos

8e604cf

921kiyo requested a review from DmitriiDeriabinQB October 8, 2020 10:10

DmitriiDeriabinQB reviewed Oct 12, 2020

View reviewed changes

921kiyo mentioned this pull request Oct 12, 2020

[KED-1696] Drop Kedro 0.14.* support #277

Merged

6 tasks

921kiyo and others added 3 commits October 12, 2020 15:09

Apply suggestions from code review

999be1e

Co-authored-by: Dmitrii Deriabin <[email protected]>

Address Dmitrii's comments

55c9402

typo

7f67108

921kiyo requested a review from DmitriiDeriabinQB October 12, 2020 16:13

limdauto reviewed Oct 12, 2020

View reviewed changes

DmitriiDeriabinQB reviewed Oct 12, 2020

View reviewed changes

package/kedro_viz/server.py Outdated Show resolved Hide resolved

call load before if statement

fe0efd8

921kiyo requested a review from DmitriiDeriabinQB October 12, 2020 19:22

DmitriiDeriabinQB approved these changes Oct 14, 2020

View reviewed changes

921kiyo merged commit 7073653 into main Oct 14, 2020

921kiyo deleted the feature/parameter-endpoint branch October 14, 2020 14:06

richardwestenra mentioned this pull request Oct 20, 2020

3.6.0 #287

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KED-2035] Expose parameter metadata #275

[KED-2035] Expose parameter metadata #275

921kiyo commented Oct 6, 2020

richardwestenra commented Oct 7, 2020

921kiyo commented Oct 7, 2020 •

edited

Loading

limdauto left a comment

richardwestenra commented Oct 7, 2020 •

edited

Loading

921kiyo commented Oct 7, 2020 •

edited

Loading

richardwestenra commented Oct 7, 2020

DmitriiDeriabinQB Oct 12, 2020

DmitriiDeriabinQB Oct 12, 2020

921kiyo Oct 12, 2020 •

edited

Loading

DmitriiDeriabinQB Oct 12, 2020

richardwestenra Oct 12, 2020

921kiyo Oct 12, 2020

DmitriiDeriabinQB Oct 12, 2020 •

edited

Loading

limdauto Oct 12, 2020

921kiyo Oct 12, 2020

DmitriiDeriabinQB Oct 12, 2020

[KED-2035] Expose parameter metadata #275

[KED-2035] Expose parameter metadata #275

Conversation

921kiyo commented Oct 6, 2020

Description

Development notes

QA notes

Checklist

Legal notice

richardwestenra commented Oct 7, 2020

921kiyo commented Oct 7, 2020 • edited Loading

limdauto left a comment

Choose a reason for hiding this comment

richardwestenra commented Oct 7, 2020 • edited Loading

921kiyo commented Oct 7, 2020 • edited Loading

richardwestenra commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

921kiyo Oct 12, 2020 • edited Loading

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

DmitriiDeriabinQB Oct 12, 2020 • edited Loading

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

921kiyo commented Oct 7, 2020 •

edited

Loading

richardwestenra commented Oct 7, 2020 •

edited

Loading

921kiyo commented Oct 7, 2020 •

edited

Loading

921kiyo Oct 12, 2020 •

edited

Loading

DmitriiDeriabinQB Oct 12, 2020 •

edited

Loading