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

DOCS-1291: Add env vars to module conf #2102

Merged
merged 10 commits into from
Nov 14, 2023

Conversation

andf-viam
Copy link
Contributor

@andf-viam andf-viam commented Oct 26, 2023

Add docs for configuring environment variables in module configuration.

  • Supports static string vars, and referencing system env vars.
  • Adds 4 new default env variables for registry modules
  • Not discussing local modules yet
  • No UI yet, so Raw JSON only.

Thanks for the helpful review, Zach!!

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Oct 26, 2023
@viambot
Copy link
Member

viambot commented Oct 26, 2023

Overall readability score: 55.66 (🟢 +0)

File Readability
configure.md 66.81 (🔴 -0.93)
View detailed metrics

🟢 - Shows an increase in readability
🔴 - Shows a decrease in readability

File Readability FRE GF ARI CLI DCRS
configure.md 66.81 45.56 8.33 12.5 11.89 5.66
  🔴 -0.93 🔴 -0.2 🔴 -0.23 🔴 -0.1 🔴 -0.12 🔴 -0.06

Averages:

  Readability FRE GF ARI CLI DCRS
Average 55.66 47.14 10.56 13.02 12.05 7.72
  🟢 +0 🟢 +0 🟢 +0 🟢 +0 🟢 +0 🟢 +0
View metric targets
Metric Range Ideal score
Flesch Reading Ease 100 (very easy read) to 0 (extremely difficult read) 60
Gunning Fog 6 (very easy read) to 17 (extremely difficult read) 8 or less
Auto. Read. Index 6 (very easy read) to 14 (extremely difficult read) 8 or less
Coleman Liau Index 6 (very easy read) to 17 (extremely difficult read) 8 or less
Dale-Chall Readability 4.9 (very easy read) to 9.9 (extremely difficult read) 6.9 or less

@andf-viam andf-viam marked this pull request as ready for review October 26, 2023 17:05
Copy link
Collaborator

@npentrel npentrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andf-viam
Copy link
Contributor Author

Hi @zaporter-work , let me know if you'd like to see any changes! Thanks!

Copy link
Member

@zaporter-work zaporter-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! This is great!

Main points:

  • Due to rewriting most of the frontend in a new framework, it doesn't make a ton of sense for me to write the UI for this just yet. It will be in the new FE and we can document it then.
  • I think it is worth documenting the default environment variables. Most importantly, it makes sense for people writing python modules to install their venv in $VIAM_MODULE_DATA. That way, their robots can re-use dependencies across module-versions.

Comment on lines 142 to 150
You can configure environment variables for a module in two ways:

- As a static value that does not change during runtime.
- As a reference to a system environment variable that can be changed during runtime.
Any system environment variable available to the `viam-server` instance can be referenced in this way.

Using static values is suitable for instances where a configuration does not need to change.
Referencing environment variables allows you to change the value of that variable at any point, effectively changing the behavior of the modular resource, without needing to restart `viam-server` for the change to take effect.
For example, you could configure an environment variable to control the hue tolerance for a [color detector](/services/vision/detection/), and decide to change that value later based on the ambient brightness of the sun.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a bit of confusion here.

Environment variables are all just strings, however, the values in these strings will be replaced using the standard viam-server placeholder replacement logic (ex: ${packages.ml_model.foo} gets replaced with the location of foo on the computer, and ${environment.PATH} gets replaced with viam-servers path variable. This is useful if you want to add something to the PATH ex: "PATH":"~/go/bin:${environment.PATH}" (which is the standard way to append environment variables))

Comment on lines 157 to 162
1. Navigate to the **Config** tab of your robot's page in [the Viam app](https://app.viam.com).
1. Click on the **Modules** subtab.
1. In the **Environment variables** section, set a **Name** for your environment variable, provide its **Value**, and click the **Add environment variable** button.

- If providing a static value to your environment variable, enter the value directly.
- If referencing a system environment variable, use the following syntax, where `MY_ENV_VAR_NAME` is the name of the system environment variable you wish to reference:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Navigate to the **Config** tab of your robot's page in [the Viam app](https://app.viam.com).
1. Click on the **Modules** subtab.
1. In the **Environment variables** section, set a **Name** for your environment variable, provide its **Value**, and click the **Add environment variable** button.
- If providing a static value to your environment variable, enter the value directly.
- If referencing a system environment variable, use the following syntax, where `MY_ENV_VAR_NAME` is the name of the system environment variable you wish to reference:

Due to weird app-timing, I am planning on pushing the ui out to the R2D2 project. Lets omit this until then (if that makes sense with you..?)

Comment on lines 182 to 183
"MY_VAR": "${environment.<MY_ENV_VAR_NAME>}",
"MY_OTHER_VAR": "<static-value>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"MY_VAR": "${environment.<MY_ENV_VAR_NAME>}",
"MY_OTHER_VAR": "<static-value>"
"MY_VAR": "<some-val>",
"PATH": "<example-folder>:${environment.PATH}"

(more concrete examples?)

Comment on lines 220 to 262
To delete an environment variable configuration, click the trash can icon to the left of the variable entry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
To delete an environment variable configuration, click the trash can icon to the left of the variable entry.

Sorry about you writing this and then this getting pushed till later!

@andf-viam andf-viam force-pushed the DOCS-1291-env-var-placeholders branch from 801309b to 4f0ed3b Compare November 7, 2023 00:32
Copy link
Member

@zaporter-work zaporter-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Probably the last pass!

Thank you so much for your time <3

Comment on lines 226 to 230
Module environment variables can be either:

- Static string values, or
- References to a system environment variable.
Any system environment variable available to the `viam-server` instance can be referenced in this way.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this distinction is necessary. I suspect users will understand how this works and this can be shortened a bit.

However, if you think this isn't intuitive and needs more explanation, I am happy to be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and I mostly mention this again on line 264. I'll remove this here. 👍

Comment on lines 262 to 265
This configures a module environment variable `PATH` that uses your system's `PATH` (which you can view by running `echo $PATH`) as a base, and adds one additional filesystem path: <file>/home/username/bin</file>.

The notation `${environment.<ENV-VAR-NAME>}"` can be used to access any system environment variable that `viam-server` has access to, where `<ENV-VAR-NAME>` represents a system environment variable, like `PATH`, `USER`, or `PWD`.
For example, you can use `${environment.HOME}"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines 292 to 314
The following is an example configuration for the [Viam ROS2 Integration module](https://app.viam.com/module/viam-soleng/viam-ros2-integration).
The configuration adds three environment variables required by the module: `PATH`, `ROS_SETUP`, and `MODULE_USER`:

- `PATH` is sourced from the local system environment variable `PATH`, and is appended with the additional search path `/home/viam/scripts`.
- `ROS_SETUP` and `MODULE_USER` are configured with static values.

```json {class="line-numbers linkable-line-numbers"}
{
"modules": [
{
"type": "registry",
"name": "my-ros2-integration",
"module_id": "viam:viam-ros2-integration",
"version": "1.0.0",
"env": {
"PATH": "${environment.PATH}:/home/viam/scripts",
"ROS_SETUP": "/opt/ros/kinetic/setup.bash",
"MODULE_USER": "my-ros-user"
}
}
]
}
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The following is an example configuration for the [Viam ROS2 Integration module](https://app.viam.com/module/viam-soleng/viam-ros2-integration).
The configuration adds three environment variables required by the module: `PATH`, `ROS_SETUP`, and `MODULE_USER`:
- `PATH` is sourced from the local system environment variable `PATH`, and is appended with the additional search path `/home/viam/scripts`.
- `ROS_SETUP` and `MODULE_USER` are configured with static values.
```json {class="line-numbers linkable-line-numbers"}
{
"modules": [
{
"type": "registry",
"name": "my-ros2-integration",
"module_id": "viam:viam-ros2-integration",
"version": "1.0.0",
"env": {
"PATH": "${environment.PATH}:/home/viam/scripts",
"ROS_SETUP": "/opt/ros/kinetic/setup.bash",
"MODULE_USER": "my-ros-user"
}
}
]
}
```

They have not had the chance to update their module to use this feature yet, and I suspect it might be better to just link to their module. (if only they put example configs in their readme's ...)

Up to you if you want to link to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep an example JSON conf, so I've made this generic instead of populating from the ROS2 integration module. 👍

<!-- prettier-ignore -->
| Name | Description |
| ---- | ----------- |
| `VIAM_HOME` | The root of the `viam-server` configuration.<br>Is always: `$HOME/.viam` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried about "Is always" because this will be changing to /opt/viam in the next few months.

The whole point of these environment variables is that module developers /should not need/ to know where VIAM_HOME is on the computer, just that it is there. But maybe it is helpful to tell users where the data is..?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about the upcoming agent change. I'll switch the language to Default:, but otherwise think keeping it is indeed valuable.

| ---- | ----------- |
| `VIAM_HOME` | The root of the `viam-server` configuration.<br>Is always: `$HOME/.viam` |
| `VIAM_MODULE_ROOT` | The root of the module install directory. Useful for file navigation that is relative to the root of the module.<br>`$VIAM_HOME/packages/.data/modules/verxxxx-my-module/` |
| `VIAM_MODULE_DATA` | A persistent folder location a module can use to store data across reboots and versions. Use this directory to store [python virtual environments](/program/python-venv/) when writing and deploying multiple python modules that share the same dependencies, for example.<br>`$VIAM_HOME/module-data/my-namespace_my-module/` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `VIAM_MODULE_DATA` | A persistent folder location a module can use to store data across reboots and versions. Use this directory to store [python virtual environments](/program/python-venv/) when writing and deploying multiple python modules that share the same dependencies, for example.<br>`$VIAM_HOME/module-data/my-namespace_my-module/` |
| `VIAM_MODULE_DATA` | A persistent folder location a module can use to store data across reboots and versions. This is a good place to store [python virtual environments](/program/python-venv/). <br>`$VIAM_HOME/module-data/cloud-robot-id/my-module-name/` |

| `VIAM_MODULE_DATA` | A persistent folder location a module can use to store data across reboots and versions. Use this directory to store [python virtual environments](/program/python-venv/) when writing and deploying multiple python modules that share the same dependencies, for example.<br>`$VIAM_HOME/module-data/my-namespace_my-module/` |
| `VIAM_MODULE_ID` | The module ID of the module. <br>`viam:realsense` |

If you are using a [local module](#local-modules), you would need to set these variables manually if your module requires them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you are using a [local module](#local-modules), you would need to set these variables manually if your module requires them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these fields except VIAM_MODULE_ROOT are also present on local modules

| Name | Description |
| ---- | ----------- |
| `VIAM_HOME` | The root of the `viam-server` configuration.<br>Is always: `$HOME/.viam` |
| `VIAM_MODULE_ROOT` | The root of the module install directory. Useful for file navigation that is relative to the root of the module.<br>`$VIAM_HOME/packages/.data/modules/verxxxx-my-module/` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `VIAM_MODULE_ROOT` | The root of the module install directory. Useful for file navigation that is relative to the root of the module.<br>`$VIAM_HOME/packages/.data/modules/verxxxx-my-module/` |
| `VIAM_MODULE_ROOT` | The root of the module install directory. Useful for file navigation that is relative to the root of the module. This is not present on local modules and you have to set it manually if your module uses it. <br>`$VIAM_HOME/packages/.data/modules/verxxxx-my-module/` |

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I like this way better. Thank you!

Copy link
Member

@zaporter-work zaporter-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I'll reply here (or on slack) once the last change is merged into the rdk and this is ready to be released.

@zaporter-work
Copy link
Member

Requisite code was just merged into the RDK and will go out during the release tomorrow.

@andf-viam
Copy link
Contributor Author

andf-viam commented Nov 8, 2023

And the award for best communicator goes to Zack!! Thank you, I'll merge tomorrow after next Tuesday's RDK release! 🎉

@andf-viam andf-viam force-pushed the DOCS-1291-env-var-placeholders branch from 6de8ef2 to 2ae01c0 Compare November 14, 2023 21:25
@viambot
Copy link
Member

viambot commented Nov 14, 2023

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/2102

@andf-viam andf-viam merged commit 5364066 into viamrobotics:main Nov 14, 2023
9 of 10 checks passed
@andf-viam andf-viam deleted the DOCS-1291-env-var-placeholders branch November 14, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants