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

RSDK-5109 - add close function #458

Merged

Conversation

purplenicole730
Copy link
Member

@purplenicole730 purplenicole730 commented Oct 9, 2023

Jira ticket

Add a close() function to the Python SDK so that users can specify what happens when a resource closes. This function is not meant to be called directly, but rather to be called when a module or resource manager is removing a resource.

Changes

  • Added a default close function in ResourceBase that returns nothing, but can be overwritten
  • Modules also now call close() when removing a resource
  • Unittests added to assert that both default (summation) and overwritten (gizmo) close() functions were called appropriately
  • Resource manager now has a close() function, where it calls all its resources' close() functions
  • Unittests added to assert that resources' closed functions were called when resource manager's close function called
  • Added close functions to most of the examples with a comment explaining that it was completely optional

Closing the resource manager was also tested manually by printing out statements in the example's server.py

@purplenicole730 purplenicole730 marked this pull request as ready for review October 11, 2023 16:36
@purplenicole730 purplenicole730 requested a review from a team as a code owner October 11, 2023 16:36
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

looks pretty good! can you update the docs/example modules to refer to this new functionality as well? In the example modules, I think just logging something will be good, just want to surface the function within the examples

src/viam/resource/manager.py Outdated Show resolved Hide resolved
src/viam/module/module.py Outdated Show resolved Hide resolved
Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

Looks good pending updates from @cheukt's comments

Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

a few smaller nits

src/viam/resource/manager.py Outdated Show resolved Hide resolved
src/viam/rpc/server.py Outdated Show resolved Hide resolved
docs/examples/module_step2_optional.py Outdated Show resolved Hide resolved
docs/examples/module_step2_optional.py Outdated Show resolved Hide resolved
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

LGTM

src/viam/rpc/server.py Outdated Show resolved Hide resolved
src/viam/resource/manager.py Outdated Show resolved Hide resolved
@purplenicole730 purplenicole730 merged commit a372745 into viamrobotics:main Oct 12, 2023
10 checks passed
@purplenicole730 purplenicole730 deleted the RSDK-5109-add-close-function branch October 16, 2023 14:46
@purplenicole730 purplenicole730 restored the RSDK-5109-add-close-function branch October 16, 2023 14:46
@purplenicole730 purplenicole730 deleted the RSDK-5109-add-close-function branch October 16, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants