-
Notifications
You must be signed in to change notification settings - Fork 9
[bricks] model configuration - refactoring for list of variables #21
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
base: main
Are you sure you want to change the base?
Conversation
dido18
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would split the PR.
- PR#1. [NO-ISSUE] Remove the
model_configuration_variablesfrom thebrick-config.yaml. The arduino-app-cli does not read/use it. - PR#2. [BREAKING] Move the
model_configurationunder themodel-list.yamland put the configuration variable for each brick. This is a breaking change. - PR#X [Enhancement] When needed, open PR to add the boolean field into the
brick_config.yaml. Each addition should be discussed and clearly described
private: true => the variable is only used internally (i.e., the model path)
mandatory: true => if the variable must be filled
secret: true => a variable that is a secret.
I do not agree. |
| - name: EI_AUDIO_CLASSIFICATION_MODEL | ||
| description: Path to the model file | ||
| private: true | ||
| - name: CUSTOM_MODEL_PATH | ||
| description: Path to the custom model directory | ||
| private: true | ||
| - name: BIND_ADDRESS | ||
| description: Bind address | ||
| private: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly think we could just remove those variables. I would avoid to add the private concepts if it isn't needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not agree. Thsi are useful and the concept of private is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? Why are they useful? Those are set by the model, so I am not sure why they should be defined here.
| requires_container: true | ||
| requires_model: true | ||
| model: mobilenet-image-classification | ||
| model_configuration_variables: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could remove the model_configuration_variables .
Is this a leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was left intentionally after last meeting in which discussed this.
Other proposal was to have this as attribute of the below variable list.
Allow to specify a per brick configuration in model's list file.
Migrate compose files variable definition inside bricks config to allow to specify more metadata