Skip to content
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

add per node data fails for multi model when using API #103

Closed
tsantosh1098 opened this issue Jul 12, 2024 · 9 comments
Closed

add per node data fails for multi model when using API #103

tsantosh1098 opened this issue Jul 12, 2024 · 9 comments
Assignees
Labels
back-end For issues where the root is mostly occurring on the back-end and not a specific adapter front-end For issues where the root is mostly occurring on the front-end priority 1 This issue is being worked on and will likely be completed by the next release stat:awaiting model-navigator The issue is actively being worked on by our navigators type:bug Bug

Comments

@tsantosh1098
Copy link

Hi,

I am trying to run the script that will load 2 models and add the additional layer info to both the models using Model explorer APIs.

Here is my code,

model_path1 = 's_mnist_quant_dyn_small.tflite'
model_path2 = 'simple_sequential_cnn.onnx'

config = model_explorer.config()

# Add model and custom node data to it
config.add_model_from_path(model_path2)
config.add_model_from_path(model_path1)

config.add_node_data_from_path('onnx_footprint.json')
config.add_node_data_from_path('tf_footprint.json')

# Visualize
model_explorer.visualize_from_config(config)

The Model explorer is adding the additionaly per node data to the first model in the list of models that model explorer take. Even though I provide the json for second model its not considered.

Do you thing something is wrong in the above code ?

@pkgoogle pkgoogle self-assigned this Jul 15, 2024
@pkgoogle
Copy link
Contributor

Hi @tsantosh1098, your code looks correct, can you provide your example models and .json node data (Or at least show screenshots of what is happening/showing)? That will help us reproduce this issue. It looks like both model paths and the node data are managed via lists, and I would expect them to map to each other in order: https://github.com/google-ai-edge/model-explorer/blob/main/src/server/package/src/model_explorer/config.py#L39

@pkgoogle pkgoogle added status:awaiting user response This label needs to be added to stale issues and PRs. status:more data needed This label needs to be added to stale issues and PRs. labels Jul 15, 2024
@tsantosh1098
Copy link
Author

Hi @pkgoogle , Thanks for the response.

Here is the updated code,

import model_explorer
from model_explorer import node_data_builder as ndb

# List of model paths
model_path1 = 'model1.tflite'
model_path2 = 'model2.tflite'


model_data1 = 'model1.json'
model_data2 = 'model2.json'

config = model_explorer.config()

# Add model and custom node data to it
config.add_model_from_path(model_path1)
config.add_model_from_path(model_path2)

config.add_node_data_from_path(model_data1)
config.add_node_data_from_path(model_data2)
# Visualize
model_explorer.visualize_from_config(config)

here are the model and json files - https://drive.google.com/drive/folders/1LpfgTTV7wdqgh4_nKlEkDdM9V3QzK_yj?usp=sharing

@pkgoogle
Copy link
Contributor

Oh I see, it seems to add both node data to just the first visualization:
image

image

So I think the problem is there's not a clear way in the config to add node data provider to a specific model visualization. Whichever part of the code is reading it assumes all the node data provider sources apply only to the first model source, so we need to change the data structure to allow for different cases and then read from it properly.

@pkgoogle pkgoogle added type:bug Bug and removed status:awaiting user response This label needs to be added to stale issues and PRs. status:more data needed This label needs to be added to stale issues and PRs. labels Jul 17, 2024
@pkgoogle
Copy link
Contributor

pkgoogle commented Jul 18, 2024

Example URL for this case:

http://localhost:8080/?data={"models":[{"url":"/path/to/model-explorer/issues/103/model1.tflite","adapterId":"builtin_tflite_flatbuffer"},{"url":"/path/to/model-explorer/issues/103/model2.tflite","adapterId":"builtin_tflite_flatbuffer"}],"nodeData":["/path/to/model-explorer/issues/103/model1.json","/path/to/model-explorer/issues/103/model2.json"],"uiState":{"paneStates":[{"deepestExpandedGroupNodeIds":[],"selectedNodeId":"","selectedGraphId":"main","selectedCollectionLabel":"model1.tflite","widthFraction":1,"selected":true,"flattenLayers":false}]}}&renderer=webgl&show_open_in_new_tab=0&enable_subgraph_selection=0

The nodeData either needs to be mapped by index or we need to create a nested nodeData structure that is associated for each model. Seems like this will most likely require changes to both backend and frontend.

Hi @jinjingforever, can you please take a look thanks.

@pkgoogle pkgoogle added front-end For issues where the root is mostly occurring on the front-end back-end For issues where the root is mostly occurring on the back-end and not a specific adapter labels Jul 18, 2024
@pkgoogle pkgoogle added the stat:awaiting model-navigator The issue is actively being worked on by our navigators label Jul 18, 2024
@santoshkumarmcw
Copy link

santoshkumarmcw commented Jul 19, 2024

@pkgoogle Thanks for the taking out time and investigating the issue. Meanwhile, I modified the backend to create similar Url. But I was not able to get any content from the frontend. As you mentioned in your above message, we might need some changes in the frontend side to visualize the model in a new URL.

Thanks

@pkgoogle
Copy link
Contributor

Yeah that URL is customized from my environment and is not URL encoded, to replicate you will need to follow the original instructions above.

@jinjingforever
Copy link
Collaborator

Thanks for the report! It is indeed an issue that all the node data added from add_node_data_from_path api call would add to the first graph. I will explore some fixes this week. Thanks!

@pkgoogle pkgoogle added the priority 1 This issue is being worked on and will likely be completed by the next release label Jul 22, 2024
@jinjingforever
Copy link
Collaborator

I've submitted the fix, and will push a new release soon.

Now you can use the new model_name parameter to specify which model to attach the node data to:

config.add_node_data_from_path(model_data1, model_name='model1.tflite')
config.add_node_data_from_path(model_data2, model_name='model2.tflite')

One thing to note is that: I needed to update model2.json to remove the top-level "main" key. It now looks like the following which should still work. The reason is that Model Explorer automatically de-dup the model graph names so the "main" graph in model2.tflite becomes "main (2)" if you load it with model1.tflite together. To make the json file work in all cases, you could remove the graph name in this case.

{
  "results": {
    "0": {
      "value": 41248
    },
    "1": {
      "value": 47040
    },
    "2": {
      "value": 18824
    },
    "3": {
      "value": 310976
    },
    "4": {
      "value": 1832
    },
    "5": {
      "value": 80
    }
  },
  "thresholds": [],
  "gradient": [
    {
      "stop": 0,
      "bgColor": "green"
    },
    {
      "stop": 0.5,
      "bgColor": "yellow"
    },
    {
      "stop": 1,
      "bgColor": "red"
    }
  ]
}

I tried the example stored in drive and it works correctly now. Thanks for the sample data and models!

Screenshot 2024-07-22 at 1 33 34 PM Screenshot 2024-07-22 at 1 34 25 PM

@santoshkumarmcw
Copy link

Great, Thanks @jinjingforever @pkgoogle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end For issues where the root is mostly occurring on the back-end and not a specific adapter front-end For issues where the root is mostly occurring on the front-end priority 1 This issue is being worked on and will likely be completed by the next release stat:awaiting model-navigator The issue is actively being worked on by our navigators type:bug Bug
Projects
None yet
Development

No branches or pull requests

4 participants