DATA-1813 Add example modular python component for filtering data capture #2839
Conversation
viambot
added
the
safe to test
| Pillow==10.0.0 | ||
| protobuf==4.24.0 | ||
| typing_extensions==4.7.1 | ||
| /Users/katherinewu/viam/viam-python-sdk |
There was a problem hiding this comment.
This is necessary to pull in recent changes to the python SDK. Once these changes are included in a release this can be changed to viam-python-sdk
There was a problem hiding this comment.
Nice. Once viam-python-sdk changes have been merged, is all that's being installed then viam-python-sdk and then the package manager will handle all the implicit dependencies specified here?
There was a problem hiding this comment.
Yes I believe so! At the end requirements.txt would just need to contain viam-python-sdk.
viambot
added
safe to test
viambot
added
safe to test
|
|
||
| # Filters the output of the underlying camera | ||
| async def get_image(self, mime_type: str = "", *, extra: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, **kwargs) -> Image.Image: | ||
| if extra and extra["fromDataManagement"]: |
There was a problem hiding this comment.
Wondering if we want to have this fromDataManagement string as an importable variable in the SDKs as well? The only reason why I'm hesitating is because I don't want to add too many variables in the SDK that they then need to keep track of across languages, and because it would be easy for a user to debug any issues here (in contrast to NoCaptureToStoreError which is checked in the server code).
There was a problem hiding this comment.
Hm, that's a great question. I see the validity of keeping track of this variable at the SDK level. Tbf, you are asking for hopefully only two variables to be kept track of. What is the process for keeping track of these "special" variables? Is it manual?
There was a problem hiding this comment.
Yep, it'll be manual for the SDK team to make sure these are consistent across sdks (currently just python and C++ since they're the only ones supporting modular components but will be more in the future) unless we build something special to track this, which falls outside the scope of this project. @benjirewis any thoughts on this?
There was a problem hiding this comment.
I don't have a ton of context here; are you asking whether this "'special" variable fromDataManagement that's currently just encoded in the extra field should be a full parameter for get_image? I suppose I would prefer an actual parameter (which would require an API change if I'm not mistaken), but if this was purely an internally-specified variable only used by the data management team than I could maybe see the justification for leaving it like this. If a user ever needs to specify fromDataManagement, then a full parameter seems pretty necessary IMO.
There was a problem hiding this comment.
Not a full parameter (which would indeed be an API change and I believe a lot of extra work on the sdk side since fromDM is currently designed to be passed through the extra map for all collectors), but rather I was thinking of a variable that users could import - something like what we can do with the go equivalent.
There was a problem hiding this comment.
flyby but I think adding something like from_dm_from_extra in a utils files might be valuable, like how we have opid_from_metadata in the python sdk. (in the sense that it's a bit manual and easy to get wrong but adding the helper reassures users that it's not that sketch and is the suggested way of accessing the var)
There was a problem hiding this comment.
Gotcha. If there's precedence in the Go module, I would also use a constant here. I see that Go uses a context key, but adding to the extra param seems ok for Python.
There was a problem hiding this comment.
Cool, thanks for the suggestions! I'll make a quick PR for this in the python SDK. And whoops sorry @benjirewis - I linked the wrong code pointer to my prev comment (fixed now)! The go camera is the only instance that needs to use the context key; the others should use extra[data.FromDMString]
| Pillow==10.0.0 | ||
| protobuf==4.24.0 | ||
| typing_extensions==4.7.1 | ||
| /Users/katherinewu/viam/viam-python-sdk |
There was a problem hiding this comment.
Nice. Once viam-python-sdk changes have been merged, is all that's being installed then viam-python-sdk and then the package manager will handle all the implicit dependencies specified here?
| VENV_NAME="venv" | ||
| PYTHON="$VENV_NAME/bin/python" | ||
|
|
||
| python3 -m venv $VENV_NAME |
There was a problem hiding this comment.
This is is a nice helper, but I actually feel like the details of how the user sets up their environment (like deciding what package manager they use, how they want to set up their environment) probably doesn't need to be defined and should be left up to them.
There was a problem hiding this comment.
Makes sense - this code was pretty much copied from the modular resource docs so can leave this file out of the example code.
There was a problem hiding this comment.
Ah actually thinking on this - I still feel like it could be good to have this here as a base so users have an example that works entirely out of the box that they can then edit to change the environment, etc. What do you think?
There was a problem hiding this comment.
Yup, after you linked to the docs, I started thinking about this more as well. Since the module need to be executed, we can/probably should have this here. Can we add just add a new add the end of the file to say the shell script needs to be executable (you need to run sudo chmod +x run.sh)
| # Filters the output of the underlying camera | ||
| async def get_image(self, mime_type: str = "", *, extra: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, **kwargs) -> Image.Image: | ||
| if extra and extra["fromDataManagement"]: | ||
| img = await self.actual_cam.get_image() |
There was a problem hiding this comment.
Small rearrangement: What do you think about moving the img = await self.actual_cam.get_image() to outside the conditional, then within the fromDataManagement check have if len(detections) == 0: raise NoCaptureToStoreError() and finally if none of those conditions are met having return img. I think this might emphasize when we're raising the error a bit better.
| from viam.services.vision import Vision | ||
|
|
||
| class ColorFilterCam(Camera, Reconfigurable): | ||
| # A ColorFilterCam wraps the underlying camera `actual_cam` and only keeps the data captured on the actual camera if `vision_service` |
There was a problem hiding this comment.
If you use triple quotes """ for the comment, it will show up in the doc string, which is probably what you want for this comment that specifies what's happening within this class
| async def get_properties(self, *, timeout: Optional[float] = None, **kwargs) -> Camera.Properties: | ||
| return await self.actual_cam.get_properties() | ||
|
|
||
| # Filters the output of the underlying camera |
There was a problem hiding this comment.
For this and all other methods, we can move the comment to inside the function definition and use """. If you want to specify the args, you could do something like:
`""" Filters the output of the underlying camera
Args: mime_type: specifies the mime_type of the data from the underlying camera ...
"""`
There was a problem hiding this comment.
Would using """ turn this into a docstring like you mentioned above? These methods are already documented in the python SDK docs, not sure if we need to do that again here - I'll move the comments as they are inside the function definition, but let me know if that's not python-y!
There was a problem hiding this comment.
It would create docstring! I'm not sure if subclassing preserves docstrings from the superclass, but I would guess not. I do think it would be helpful to include the docstring if nothing else then for this method since this is the one thing that's different than the usual python SDK stuff. I think moving comments inside the function definition is python-y! :)
There was a problem hiding this comment.
Sounds good, added docstrings to the implemented methods (the unimplemented ones are self-explanatory)
|
|
||
| # Filters the output of the underlying camera | ||
| async def get_image(self, mime_type: str = "", *, extra: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, **kwargs) -> Image.Image: | ||
| if extra and extra["fromDataManagement"]: |
There was a problem hiding this comment.
Hm, that's a great question. I see the validity of keeping track of this variable at the SDK level. Tbf, you are asking for hopefully only two variables to be kept track of. What is the process for keeping track of these "special" variables? Is it manual?
| async def get_properties(self, *, timeout: Optional[float] = None, **kwargs) -> Camera.Properties: | ||
| return await self.actual_cam.get_properties() | ||
|
|
||
| # Filters the output of the underlying camera |
There was a problem hiding this comment.
Would using """ turn this into a docstring like you mentioned above? These methods are already documented in the python SDK docs, not sure if we need to do that again here - I'll move the comments as they are inside the function definition, but let me know if that's not python-y!
|
|
||
| # Filters the output of the underlying camera | ||
| async def get_image(self, mime_type: str = "", *, extra: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, **kwargs) -> Image.Image: | ||
| if extra and extra["fromDataManagement"]: |
There was a problem hiding this comment.
Yep, it'll be manual for the SDK team to make sure these are consistent across sdks (currently just python and C++ since they're the only ones supporting modular components but will be more in the future) unless we build something special to track this, which falls outside the scope of this project. @benjirewis any thoughts on this?
| # Filters the output of the underlying camera | ||
| async def get_image(self, mime_type: str = "", *, extra: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, **kwargs) -> Image.Image: | ||
| if extra and extra["fromDataManagement"]: | ||
| img = await self.actual_cam.get_image() |
| Pillow==10.0.0 | ||
| protobuf==4.24.0 | ||
| typing_extensions==4.7.1 | ||
| /Users/katherinewu/viam/viam-python-sdk |
There was a problem hiding this comment.
Yes I believe so! At the end requirements.txt would just need to contain viam-python-sdk.
| VENV_NAME="venv" | ||
| PYTHON="$VENV_NAME/bin/python" | ||
|
|
||
| python3 -m venv $VENV_NAME |
There was a problem hiding this comment.
Makes sense - this code was pretty much copied from the modular resource docs so can leave this file out of the example code.
viambot
added
safe to test
|
Adding @benjirewis to this PR since Naveed is OOO. Two main questions:
|
|
Looking at the examples you linked to above -- I feel like this should go in the Python SDK examples |
|
|
||
| # Filters the output of the underlying camera | ||
| async def get_image(self, mime_type: str = "", *, extra: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, **kwargs) -> Image.Image: | ||
| if extra and extra["fromDataManagement"]: |
There was a problem hiding this comment.
I don't have a ton of context here; are you asking whether this "'special" variable fromDataManagement that's currently just encoded in the extra field should be a full parameter for get_image? I suppose I would prefer an actual parameter (which would require an API change if I'm not mistaken), but if this was purely an internally-specified variable only used by the data management team than I could maybe see the justification for leaving it like this. If a user ever needs to specify fromDataManagement, then a full parameter seems pretty necessary IMO.
|
The Python SDK examples currently have the simple and complex examples, which we think is probably enough for now 🤔 |
|
Taking myself off this one since it seems we have plenty of reviewers - feel free to add me back though @kaywux if you need another set of eyes! |
viambot
removed
the
safe to test
viambot
added
the
safe to test
|
Updated the code to use the newly-added SDK util function (thanks @benjirewis for a quick review!) - once this PR is LGTM'ed I am planning to move this example, as well as the go example code that is already in |
viambot
added
safe to test
Code Coverage
|
|
Thanks for the reviews on this! Closing this PR as the code here has been added to https://github.com/viam-labs/modular-filter-examples. |
Code example for python version of https://viam.atlassian.net/browse/DATA-1802