-
Notifications
You must be signed in to change notification settings - Fork 45
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-3699, DOCS-2540: Add module lifecycle and dependencies guidance #4156
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for viam-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -191,7 +191,7 @@ If you need to maintain the state of your resource, see [(Optional) Create and e | |||
{{< tabs >}} | |||
{{% tab name="Python" %}} | |||
|
|||
If you prefer to use explicit dependencies (for example, for an optional dependency), the steps are the same as for implicit dependencies above, except that you do not need to return the dependency from the `validate_config` method and can instead return an empty list: | |||
If you prefer to use explicit dependencies (for example, for an optional dependency), the steps are the same as for implicit dependencies, except that you do not need to return the dependency from the `validate_config` method and can instead return an empty list: |
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 that saying "for example, for an optional dependency" is wrong here. A resource will still wait for all resources declared in depends_on to be available before starting.
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.
We have two mechanisms for declaring dependencies which are essentially the same result. Implicit is more common than explicit
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.
What I mean by "optional" here is that if the user doesn't configure a "camera_name" at all, either in depends_on or in attributes, that the modular resource should still work. But I'm realizing the example I have here doesn't indicate that--I should make this validate_config explicit dependency example show "camera_name" as an optional attribute, and keep it as a required attribute for the implicit dependency example.
Is it true that people should use implicit dependencies for required things and explicit dependencies for optional things?
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 guess, is it currently possible for people to have an optional implicit dependency, or not until the upcoming work happens?
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.
No way to have an optional explicit dependency. Yeah you could make an implicit dependency optional. Its a bit annoying right now:
- See in validate that its not defined and don't return it as a dependency.
- In your implementation you check to make sure the dependency is not null and gracefully skip using it.
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.
To illuminate other reviewers: Based on offline sync, we were using "optional" in different ways--If a user does put something in depends_on, the resource won't currently start if the dependency doesn't start. What I was meaning with "optional" is it's optional for the user to even configure it at all. But sounds like you can do that with either implicit or explicit deps--it's all about how you implement validate.
Based on offline convo I will remove explicit dependencies since they just confuse users and aren't actually necessary except in cases of backwards compatibility.
{{< /tabs >}} | ||
|
||
{{% hiddencontent %}} | ||
There is not currently an SDK method to access configuration attributes of dependencies in Python or Go, but in Python it is possible to use `get_robot_part` to return information including the whole configuration of a machine part, and then access the configuration attributes of the dependency from there. |
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.
Maybe this isn't right--looking into this more
@michaellee1019 I changed it to optional vs required but LMK if you think that is still confusing or overkill and it'd be better to just have one heading with "use dependencies" in general, with the required deps examples, and omit the optional examples. |
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.
did a quick pass, I'll be out next week so no need for my approval - if we want an opinion from netcode we can add @benjirewis
{{< /tabs >}} | ||
|
||
{{% hiddencontent %}} | ||
There is not currently an SDK method to directly access configuration attributes of dependencies in Python or Go, but in Python it is possible to use `get_robot_part` to return information including the whole configuration of a machine part, and then access the configuration attributes of the dependency from there. |
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.
what does this mean? I think we would want to discourage users from looking at configs for other resources
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.
We got a chatbot question about this specifically (from Sean Yu) and the bot answer was incorrect so was hoping to fix it. This hidden content short code means it’s not visible on the page, only to the chatbot if someone specifically asks. But if we don’t want people to do this at all should I just change this to say “you can’t, period”?
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.
ok, I guess in that case this is fine - we should definitely discourage it tho
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.
This is awesome! You guys do a great job of adding stuff around this somewhat opaque logic. Some comments and I can help further while Cheuk is on PTO.
1. `viam-server` starts, and if it is connected to the internet, it checks for configuration updates. | ||
1. `viam-server` starts any configured modules. | ||
1. When a module initializes, it registers its model or models and associated [APIs](/dev/reference/apis/) with `viam-server`, making the models available for use. | ||
1. For each modular resource configured on the machine, `viam-server` uses the resource's `validate` function and the `depends_on` field in the resource configuration to determine the dependencies of the resource. |
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.
Just a note that an upcoming NetCode change may further complicate this by adding a new type of "optional" dependency https://viam.atlassian.net/browse/RSDK-10135.
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.
Yeah...I figured since the work hasn't started on implementing it it makes sense to try and get this merged with what is true now. Any idea on timeline for this change though?
1. If at any point the user changes the configuration of the machine, `viam-server` reconfigures the affected resources within 15 seconds. | ||
1. If a resource crashes, `viam-server` attempts to rebuild it. | ||
1. When `viam-server` shuts down, it attempts to stop all running modules. | ||
If a module fails to shut down within 90 seconds, `viam-server` will forcefully kill it after 15 seconds. |
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 believe it's actually:
SIGTERM
to module process- wait 10s
SIGTERM
to module process group- wait 20s
SIGKILL
to module process group
So the timings you have there are inaccurate.
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.
The SIGTERMs are on a different timer, they are only triggered when we actually get to the clean module shutdown phase.
SIGKILLs are triggered after 90s from when the restart/shutdown is started, and the SIGTERMS can happen at any time between the shutdown is started and 90s
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.
Ah I forgot about those SIGKILL
s thanks @cheukt. So, yeah, slightly complicated "killing" logic (because we can SIGKILL a module due to it taking 90s since viam-server
started shutdown or it taking 30s since viam-server
tried to start shutting down a single module, right?)
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.
ya, you're right about that - I forgot about the SIGKILL after 30s from the viam-server trying to shutdown a module.
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.
So, this?
If a module fails to shut down within 90 seconds, `viam-server` will forcefully kill it after 15 seconds. | |
If `viam-server` attempts to shut down an individual module (for example due to a user disabling a module) and the module does not shut down within 30 seconds, `viam-server` kills the module. | |
1. When `viam-server` shuts down, it first shuts down all modules and kills them if they fail to shut down within 90 seconds. |
if cfg.CameraName == "" { | ||
return nil, resource.NewConfigValidationFieldRequiredError(path, "camera_name") | ||
} | ||
if reflect.TypeOf(cfg.CameraName).Kind() != reflect.String { |
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 don't think you need this reflection check. cfg.CameraName
will definitely be a string
at this point.
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.
Does it get checked by viam-server automatically because CameraName string
specifies type?json:"camera_name"
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.
Basically yep! It's not technically viam-server
that does the conversion from proto to a golang string, but the Golang module library code does it and will error if the thing specified in JSON is not actually a string/cfg.CameraName
will just ""
.
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.
Ah okay ty!! Wait it'll error or it'll assign it an empty string or both?
Discovery service will be further fleshed out separately
Staging:
https://deploy-preview-4156--viam-docs.netlify.app/operate/get-started/other-hardware/#how-and-where-do-modules-run and https://deploy-preview-4156--viam-docs.netlify.app/operate/get-started/other-hardware/dependencies/